Fix -Wshadow=local warnings

On Tue, Sep 1, 2026 at 10:59 AM Peter Eisentraut <peter@eisentraut.org> wrote:

These can be caught if we dial up the warning one notch to
-Wshadow=local. This then flags all shadowing of a local variable by
another local variable. I have fixed all the warnings in the attached
patch. I think everything this catches is obviously bad, so this seems
well worth fixing. (And if we buy into the idea of
-Wshadow=compatible-local, then this is obviously better and more complete.)

+1 to fixing all of these, and to using -Wshadow=compatible-local.

--
Peter Geoghegan

On Sep 1, 2026, at 22:58, Peter Eisentraut <peter@eisentraut.org> wrote:

I bumped into some variable shadowings that to my slight surprise the current warning option -Wshadow=compatible-local does not catch. For example

const char *p;
char *p;

or

bool skipped;
int64 skipped;

These are not "compatible" in the technical C language sense, but they are mutually assignable, so IMO just as confusing and fragile.

Also, there are things like

EState *estate;
ExprState *estate;

which are not mutually assignable, but almost as dangerous given the propensity to cast node types around.

These can be caught if we dial up the warning one notch to -Wshadow=local. This then flags all shadowing of a local variable by another local variable. I have fixed all the warnings in the attached patch. I think everything this catches is obviously bad, so this seems well worth fixing. (And if we buy into the idea of -Wshadow=compatible-local, then this is obviously better and more complete.)

So the first patch fixes all the warnings, but doesn't turn up the compiler flag yet. There is a hiccup with the LLVM headers, because they themselves trigger these warnings. So the second patch provides a workaround to silence warnings from those headers. It's a bit different from what we have done before, but I think it works better for this case. Alternative ideas welcome. In the third patch, the warning option is then changed.
<0001-Fix-Wshadow-local-warnings.patch><0002-Use-isystem-for-LLVM-include-directories.patch><0003-Use-warning-option-Wshadow-local.patch>

I had a patch to fix all warnings from -Wshadow-all, and 0001 seems to be a subset of that patch. My patch was not accepted due to a concern about adding burden to future back-patching work. Anyway, +1 from my side for fixing these warnings.

A few small comments:

1 - 0001 - dependencies.c
```
  * expression into *expr.
  */
 static bool
-dependency_is_compatible_expression(Node *clause, Index relid, List *statlist, Node **expr)
+dependency_is_compatible_expression(Node *clause, Index relid, List *statlist, Node **stat_expr_p)
```

As “expr” is renamed, the function header comment needs to be updated as well.

2 - 0001 - pg_constraint.c
```
+ CookedConstraint *cooked_constr;
```

In the current RelationGetNotNullConstraints(), other local variables use camelCase naming, for example constrRel, so maybe it would be better to keep the naming style consistent.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On 02.09.26 07:19, Chao Li wrote:

A few small comments:

1 - 0001 - dependencies.c
```
* expression into *expr.
*/
static bool
-dependency_is_compatible_expression(Node *clause, Index relid, List *statlist, Node **expr)
+dependency_is_compatible_expression(Node *clause, Index relid, List *statlist, Node **stat_expr_p)
```

As “expr” is renamed, the function header comment needs to be updated as well.

2 - 0001 - pg_constraint.c
```
+ CookedConstraint *cooked_constr;
```

In the current RelationGetNotNullConstraints(), other local variables use camelCase naming, for example constrRel, so maybe it would be better to keep the naming style consistent.

Thanks, the patch with the code changes has been committed, with your
suggestions incorporated.

The patch to activate the option couldn't be committed yet because the
workaround for the LLVM headers didn't work and had to be reverted
(commit 7a0aa99e51c). I have worked on a new solution that works more
along the lines of how other per-file or per-module "-Wno-..." options
are handled. I think this will be simpler and less fragile. See
attached patch.