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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Tree-walker callbacks vs -Wdeprecated-non-prototype
Date: 2022-09-17 01:08:02
Message-ID: 3953550.1663376882@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Ugh. I wonder if we can get away with declaring the walker arguments
> as something like "bool (*walker) (Node *, void *)" without having
> to change all the actual walkers to be exactly that signature.
> Having to insert casts in the walkers would be a major pain-in-the-butt.

No joy on that: both gcc and clang want the walkers to be declared
as taking exactly "void *".

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.

In nodeFuncs.c, we can hide the newly-required casts inside macros;
indeed, the mutators barely need any changes because they already
had MUTATE() macros that contained casts. So on that side, it feels
to me that this is actually a bit nicer than before.

For the callers, we can either do it as I did below:

static bool
-cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
+cost_qual_eval_walker(Node *node, void *ctx)
{
+ cost_qual_eval_context *context = (cost_qual_eval_context *) ctx;
+
if (node == NULL)
return false;

or perhaps like this:

static bool
-cost_qual_eval_walker(Node *node, cost_qual_eval_context *context)
+cost_qual_eval_walker(Node *node, void *context)
{
+ cost_qual_eval_context *cqctx = (cost_qual_eval_context *) context;
+
if (node == NULL)
return false;

but the latter would require changing references further down in the
function, so I felt it more invasive.

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.

regards, tom lane

Attachment Content-Type Size
clean-up-tree-walker-warnings-wip.patch text/x-diff 45.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2022-09-17 01:20:56 Re: Making C function declaration parameter names consistent with corresponding definition names
Previous Message bt22nakamorit 2022-09-17 00:44:33 Re: Make ON_ERROR_STOP stop on shell script failure