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.
* 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
(the fact that it lacks any cast now is sloppy and not per project style,
IMO).
There were a bunch of places in ab1f0c822 where I wished I had this,
but I can go back and back-fill that later; doesn't need to be in the
first commit.
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. 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.
regards, tom lane
