On 11/11/19 2:33 PM, Mark Dilger wrote:
>
>
> On 11/11/19 1:41 PM, Tom Lane wrote:
>> Would it be a good idea to move find_expr_references_walker to
>> nodeFuncs.c, in hopes of making it more visible to people adding
>> new node types?
>
> I'm not sure that would be enough. The logic of that function is not
> immediately obvious, and where to add a node to it might not occur to
> people. If the repeated use of
>
> else if (IsA(node, XXX))
>
> were replaced with
>
> switch (nodeTag(node)) {
> case XXX:
>
> then the compiler, ala -Wswitch, would alert folks when they forget to
> handle a new node type.
>
I played with this a bit, making the change I proposed, and got lots of
warnings from the compiler. I don't know how many of these would need
to be suppressed by adding a no-op for them at the end of the switch vs.
how many need to be handled, but the attached patch implements the idea.
I admit adding all these extra cases to the end is verbose....
The change as written is much too verbose to be acceptable, but given
how many places in the code could really use this sort of treatment, I
wonder if there is a way to reorganize the NodeTag enum into multiple
enums, one for each logical subtype (such as executor nodes, plan nodes,
etc) and then have switches over enums of the given subtype, with the
compiler helping detect tags of same subtype that are unhandled in the
switch.
I have added enough nodes over the years, and spent enough time tracking
down all the parts of the code that need updating for a new node, to say
that this would be very helpful if we could make it work. I have not
done the research yet on how many places would be made less elegant by
such a change, though. I think I'll go look into that a bit....
--
Mark Dilger
