Re: safer node casting

On 2017-01-26 16:55:33 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > How about something like the attached? The first patch just contains
> > castNode(), the second one a rebased version of Peter's changes (with
> > some long lines broken up).
> 
> Looks generally good.  A couple quibbles from a quick read-through:
> 
> * All but the first change in ProcessCopyOptions seem rather pointless:
> 
>  			else if (defel->arg && IsA(defel->arg, List))
> -				cstate->force_quote = (List *) defel->arg;
> +				cstate->force_quote = castNode(List, defel->arg);
> 
> In these places, castNode() isn't checking anything the if-condition
> didn't.  Maybe it's good style anyway, but I'm not really convinced.

Agreed that it's not not necessary - I didn't add this one (or any
castNode actually). But I don't think it matters much.


> * In ExecInitAgg:
> 
>  			aggnode = list_nth(node->chain, phase - 1);
> -			sortnode = (Sort *) aggnode->plan.lefttree;
> -			Assert(IsA(sortnode, Sort));
> +			sortnode = castNode(Sort, aggnode->plan.lefttree);
> 
> it seems like the assignment to aggnode ought to have a castNode on it
> too

Yea, looks good.


> (the fact that it lacks any cast now is sloppy and not per project style,
> IMO).

There's a lot of these missing :(.  This is one of these things that'd
be a lot easier to enforce if we'd be able to compile in a c++
compatible mode (-Wc++-compat), because there void * to X * casts
have to be done explicitly.


> BTW, maybe it's just the first flush of enthusiasm, but I can see us
> using this so much that the lack of it in back branches will become
> a serious PITA for back-patching.

Might, yea.


> So I'm strongly tempted to propose
> that your 0001 should be back-patched.  However, before 9.6 we didn't
> have the compiler requirement that "static inline" in headers must be
> handled sanely.  Maybe a useful compromise would be to put 0001 in 9.6,
> and before that just add
> 
> #define castNode(_type_,nodeptr)  ((_type_ *)(nodeptr))
> 
> which would allow the notation to be used safely, albeit without
> any assertion backup.  Alternatively, we could enable the assertion
> code only for gcc, which would probably be plenty good enough for
> finding bugs in stable branches.

#if defined(USE_ASSERT_CHECKING) && defined(PG_USE_INLINE)
is probably a better gatekeeper in the back-branches, than gcc? Then we
can just remove the defined(PG_USE_INLINE) and it's associated comment
in >= 9.6.

Regards,

Andres