Horton

Re: Tree-walker callbacks vs -Wdeprecated-non-prototype

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-09-18T20:57:45Z
Lists: pgsql-hackers

Attachments

I wrote:
> Attached is an incomplete POC patch that suppresses these warnings
> in nodeFuncs.c itself and in costsize.c, which I selected at random
> as a typical caller.  I'll push forward with converting the other
> call sites if this way seems good to people.

Here's a fleshed-out patch that gets rid of all warnings of this sort
(tested on clang version 15.0.0).

While I remain happy enough with what has to be done in nodeFuncs.c,
I'm really not happy at all with this point:

> It's sad to note that this exercise in hoop-jumping actually leaves
> us with net LESS type safety, because the outside callers of
> cost_qual_eval_walker are no longer constrained to call it with
> the appropriate kind of context struct.  Thanks, C committee.

There are a lot of these walker/mutator functions and hence a whole
lot of opportunity to pass the wrong thing, not only from the outer
non-recursive call points but during internal recursions in the
walkers/mutators themselves.

I think we ought to seriously consider the alternative of changing
nodeFuncs.c about like I have here, but not touching the walkers/mutators,
and silencing the resulting complaints about function type casting by
doing the equivalent of

-    return expression_tree_walker(node, cost_qual_eval_walker,
-                                  (void *) context);
+    return expression_tree_walker(node,
+                                  (tree_walker_callback) cost_qual_eval_walker,
+                                  (void *) context);

We could avoid touching all the call sites by turning
expression_tree_walker and friends into macro wrappers that incorporate
these casts.  This is fairly annoying, in that it gives up the function
type safety the C committee wants to impose on us; but I really think
the data type safety that we're giving up in this version of the patch
is a worse hazard.

BTW, I was distressed to discover that someone decided they could
use ExecShutdownNode as a planstate_tree_walker() walker even though
its argument list is not even the right length.  I'm a bit flabbergasted
that we seem to have gotten away with that so far, because I'd have
thought for sure that it'd break some platform's convention for which
argument gets passed where.  I think we need to fix that, independently
of what we do about the larger scope of these problems.  To avoid an
API break, I propose making ExecShutdownNode just be a one-liner that
calls an internal ExecShutdownNode_walker() function.  (I've not done
it that way in the attached, though.)

Thoughts?

			regards, tom lane

Commits

  1. Remove accidentally added meson.build

  2. Disable clang 16's -Wcast-function-type-strict.

  3. Disable -Wdeprecated-non-prototype in the back branches.

  4. Revise tree-walk APIs to improve spec compliance & silence warnings.

  5. Future-proof the recursion inside ExecShutdownNode().