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