Thread: [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

On Fri, Mar 17, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de> wrote:
> 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

Kevin Grittner <kgrittn@gmail.com> writes:
> 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

On 2017-03-17 15:29:27 -0500, Kevin Grittner wrote:
> On Fri, Mar 17, 2017 at 3:23 PM, Andres Freund <andres@anarazel.de> wrote:
> > 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