> On Sep 1, 2026, at 22:58, Peter Eisentraut <[email protected]> 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/
