Re: [HACKERS] Candidate for local inline function?
On 2017-03-17 15:29:27 -0500, Kevin Grittner wrote: > On Fri, Mar 17, 2017 at 3:23 PM, Andres Freundwrote: > > On 2017-03-17 15:17:33 -0500, Kevin Grittner wrote: > >> Why do we warn of a hazard here instead of eliminating said hazard > >> with a static inline function declaration in executor.h? > > > > Presumably because it was written long before we started relying on > > inline functions :/ > > Right. git blame says it was changed in 2004. > > >> /* > >> * ExecEvalExpr was formerly a function containing a switch statement; > >> * now it's just a macro invoking the function pointed to by an ExprState > >> * node. Beware of double evaluation of the ExprState argument! > >> */ > >> #define ExecEvalExpr(expr, econtext, isNull) \ > >> ((*(expr)->evalfunc) (expr, econtext, isNull)) > >> > >> Should I change that to a static inline function doing exactly what > >> the macro does? In the absence of multiple evaluations of a > >> parameter with side effects, modern versions of gcc have generated > >> the same code for a macro versus a static inline function, at least > >> in the cases I checked. > > > > I'm absolutely not against changing this to an inline function, but I'd > > prefer if that code weren't touched quite right now, there's a large > > pending patch of mine in the area. If you don't mind, I'll just include > > the change there, rather than have a conflict? > > Fine with me. For posterities sake: I've indeed done so. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Candidate for local inline function?
Kevin Grittnerwrites: > Why do we warn of a hazard here instead of eliminating said hazard > with a static inline function declaration in executor.h? > /* > * ExecEvalExpr was formerly a function containing a switch statement; > * now it's just a macro invoking the function pointed to by an ExprState > * node. Beware of double evaluation of the ExprState argument! > */ > #define ExecEvalExpr(expr, econtext, isNull) \ > ((*(expr)->evalfunc) (expr, econtext, isNull)) > Should I change that to a static inline function doing exactly what > the macro does? No, because that code has only days to live anyway. You'd just create a merge hazard for Andres' execQual rewrite. In practice, I seriously doubt that there are or ever will be any callers passing volatile expressions to this macro, so that there's not really much advantage to be gained by assuming that the compiler is smart about inline functions. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Candidate for local inline function?
On Fri, Mar 17, 2017 at 3:23 PM, Andres Freundwrote: > On 2017-03-17 15:17:33 -0500, Kevin Grittner wrote: >> Why do we warn of a hazard here instead of eliminating said hazard >> with a static inline function declaration in executor.h? > > Presumably because it was written long before we started relying on > inline functions :/ Right. git blame says it was changed in 2004. >> /* >> * ExecEvalExpr was formerly a function containing a switch statement; >> * now it's just a macro invoking the function pointed to by an ExprState >> * node. Beware of double evaluation of the ExprState argument! >> */ >> #define ExecEvalExpr(expr, econtext, isNull) \ >> ((*(expr)->evalfunc) (expr, econtext, isNull)) >> >> Should I change that to a static inline function doing exactly what >> the macro does? In the absence of multiple evaluations of a >> parameter with side effects, modern versions of gcc have generated >> the same code for a macro versus a static inline function, at least >> in the cases I checked. > > I'm absolutely not against changing this to an inline function, but I'd > prefer if that code weren't touched quite right now, there's a large > pending patch of mine in the area. If you don't mind, I'll just include > the change there, rather than have a conflict? Fine with me. -- Kevin Grittner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Candidate for local inline function?
Hi Kevin, On 2017-03-17 15:17:33 -0500, Kevin Grittner wrote: > Why do we warn of a hazard here instead of eliminating said hazard > with a static inline function declaration in executor.h? Presumably because it was written long before we started relying on inline functions :/ > /* > * ExecEvalExpr was formerly a function containing a switch statement; > * now it's just a macro invoking the function pointed to by an ExprState > * node. Beware of double evaluation of the ExprState argument! > */ > #define ExecEvalExpr(expr, econtext, isNull) \ > ((*(expr)->evalfunc) (expr, econtext, isNull)) > > Should I change that to a static inline function doing exactly what > the macro does? In the absence of multiple evaluations of a > parameter with side effects, modern versions of gcc have generated > the same code for a macro versus a static inline function, at least > in the cases I checked. I'm absolutely not against changing this to an inline function, but I'd prefer if that code weren't touched quite right now, there's a large pending patch of mine in the area. If you don't mind, I'll just include the change there, rather than have a conflict? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Candidate for local inline function?
Why do we warn of a hazard here instead of eliminating said hazard with a static inline function declaration in executor.h? /* * ExecEvalExpr was formerly a function containing a switch statement; * now it's just a macro invoking the function pointed to by an ExprState * node. Beware of double evaluation of the ExprState argument! */ #define ExecEvalExpr(expr, econtext, isNull) \ ((*(expr)->evalfunc) (expr, econtext, isNull)) Should I change that to a static inline function doing exactly what the macro does? In the absence of multiple evaluations of a parameter with side effects, modern versions of gcc have generated the same code for a macro versus a static inline function, at least in the cases I checked. -- Kevin Grittner