Re: [HACKERS] safer node casting

Hi,

On 2017-01-03 11:00:47 +0530, Ashutosh Bapat wrote:
> On Mon, Jan 2, 2017 at 2:10 PM, Andres Freund  wrote:
> > Hi,
> >
> >
> > On 2016-12-31 12:08:22 -0500, Peter Eisentraut wrote:
> >> There is a common coding pattern that goes like this:
> >>
> >>     RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
> >>     Assert(IsA(rinfo, RestrictInfo));
> >
> >
> >> I propose a macro castNode() that combines the assertion and the cast,
> >> so this would become
> >>
> >>     RestrictInfo *rinfo = castNode(RestrictInfo, lfirst(lc));
> >
> > I'm quite a bit in favor of something like this, having proposed it
> > before ;)
> >
> >> +#define castNode(_type_,nodeptr)     (AssertMacro(!nodeptr || IsA(nodeptr,_type_)), (_type_ *)(nodeptr))
> >
> > ISTM that we need to do the core part of this in an inline function, to
> > avoid multiple evaluation hazards - which seem quite likely to occur
> > here - it's pretty common to cast the result of a function after all.
> >
> > Something like
> >
> > static inline Node*
> > castNodeImpl(void *c, enum NodeTag t)
> > {
> >     Assert(c == NULL || IsA(c, t));
> >     return c;
> > }
> >
> > #define castNode(_type_, nodeptr) ((_type_ *) castNodeImpl(nodeptr, _type_))
> >
> > should work without too much trouble afaics?
> >
> I tried this quickly as per attached patch. It gave a compiler error
> createplan.c: In function ‘castNodeImpl’:
> createplan.c:340:2: error: ‘T_t’ undeclared (first use in this function)
> createplan.c:340:2: note: each undeclared identifier is reported only
> once for each function it appears in
> createplan.c: In function ‘create_plan_recurse’:
> createplan.c:445:13: error: expected expression before ‘AggPath’

Well, I wrote that just to outline my suggestion, not as a patch ;).
It's just that we have to replace IsA() with nodeTag(nodeptr) == t
(because IsA does string concat magic).

Greetings,

Andres Freund