Re: Fix -Wshadow=local warnings

> On Sep 1, 2026, at 22:58, Peter Eisentraut <peter(at)eisentraut(dot)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/