Skip to content

Fix -Wshadow=local warnings - #398

Open
pg-hub-mirror[bot] wants to merge 1 commit into
masterfrom
pg-hub/mirror-patch-4f39c2c12d720097
Open

pg-hub-mirror[bot] wants to merge 1 commit into
masterfrom
pg-hub/mirror-patch-4f39c2c12d720097

Conversation

@pg-hub-mirror

@pg-hub-mirror pg-hub-mirror Bot commented Sep 17, 2026

Copy link
Copy Markdown

Read-only mirror. Reply and review on pgsql-hackers; activity here is not sent upstream.

  • Original author: Peter Eisentraut <peter(at)eisentraut(dot)org>
  • Mailing list: pgsql-hackers
  • Message-ID: a75a6d38-ea88-4459-ad83-e7425aa03c9a@eisentraut.org
  • Original email

Patch files:


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 7a0aa99). 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.

Change the existing -Wshadow=compatible-local to -Wshadow=local.  This
covers more cases than before.  In particular, some types are
assignable to each other, such as char * and const char *, or bool and
some integer type, but they are not "compatible" in the C sense, so
they are missed by the previous warning setting, but they are really
the same basic problem.

But: The LLVM headers are not clean for this option, so we need to
disable this option for those headers.  We do this by also detecting
in configure and meson the negative form of the warning option and
applying it when compiling the respective files, similar to how other
per-file warning disabling already works.

This technically loses the previous -Wshadow=compatible-local checking
of the LLVM-using files, but this worked only by accident anyway and
there is no guarantee that future LLVM versions wouldn't run afoul of
that warning level.

The variables are intentionally named "...shadow", not
"...shadow_local" or similar, so that other shadow warning option
variants could be substituted easily.

In the long run, a better solution might be to use -isystem for the
LLVM include directories, as was attempted in commit 704ef84 but
then reverted in 7a0aa99, because porting that logic from meson
accurately to configure is complicated and fragile and hard to test
and maintain.

Discussion: https://www.postgresql.org/message-id/flat/84b6f128-91f5-480e-8a9e-7d0e8f538cea%40eisentraut.org
@pg-hub-mirror pg-hub-mirror Bot added area:storage Storage, access methods, buffers, or I/O source:pgsql-hackers Mirrored from pgsql-hackers type:patch Mail thread contains a PostgreSQL patch labels Sep 17, 2026
@pg-hub-mirror pg-hub-mirror Bot locked and limited conversation to collaborators Sep 17, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area:storage Storage, access methods, buffers, or I/O source:pgsql-hackers Mirrored from pgsql-hackers type:patch Mail thread contains a PostgreSQL patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant