Re: [HACKERS] Candidate for local inline function?

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


-- 
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?

2017-03-17 Thread Tom Lane 
Kevin Grittner  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


-- 
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?

2017-03-17 Thread Kevin Grittner 
On Fri, Mar 17, 2017 at 3:23 PM, Andres Freund  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


-- 
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?

2017-03-17 Thread Andres Freund 
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?

2017-03-17 Thread Kevin Grittner 
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