On 2017-01-26 16:55:33 -0500, Tom Lane wrote: > Andres Freund <and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers