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. * 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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers