Hi Peter,
On Nov 29, 2025, at 23:29, Peter Eisentraut <peter@eisentraut.org> wrote:
On 28.11.25 09:16, Chao Li wrote:
Hi Hackers,
While reviewing [1], it makes me recall an experience where I had a patch ready locally, but CommitFest CI failed with a shadows-variable warning. Now I understand that -Wall doesn't by default enable -Wshadows with some compilers like clang.
I did a clean build with manually enabling -Wshadow and surprisingly found there are a lot of such warnings in the current code base, roughly ~200 occurrences.
As there are too many, I plan to fix them all in 3-4 rounds. For round 1 patch, I'd see any objection, then decide if to proceed more rounds.
See <https://www.postgresql.org/message-id/flat/20220817145434.GC26426%40telsasoft.com> for a previous long thread on this, which led to the addition of the -Wshadow=compatible-local flag.
Thanks for pointing out the previous discussion. I have read through the discussion. Looks like there was no objection on the direction of cleaning up the shadow-variable warnings, folks were discussing how to do the cleanup. I think my v1 patch has already taken the “saner” way as Micheal suggested: renaming inter local variables.
I counted all warnings, there are totally 167 occurrences, seems a manageable number. Instead of randomly splitting all fixes into several commits as v1 did, which would really discourage reviewers and committers, in v2, I tried to categorize all warnings to different types, and put fixes of different types into different commits, which should make reviews a lot easier. All commits are self-contained, each of them can be reviewed and pushed independently.
I think this is a slightly unsatisfactory state, because that is a gcc-specific option, and maybe you are using clang or something else. So maybe some further cleanup is useful, but please check the previous discussions.
I know -Wshadow=compatible-local is not supported by all compilers, some compilers may fallback to -Wshadow and some may just ignore it. This patch set has cleaned up all shadow-variable warnings, once the cleanup is done, in theory, we should be able to enable -Wshadow.
0001 - simple cases of local variable shadowing local variable by changing inner variable to for loop variable (also need to rename the for loop var)
0002 - simple cases of local variable shadowing local variable by renaming inner variable
0003 - simple cases of local variable shadowing local variable by renaming outer variable. In this commit, outer local variables are used much less than inner variables, thus renaming outer is simpler than renaming inner.
0004 - still local shadows local, but caused by a macro definition, only in inval.c
0005 - cases of global wal_level and wal_segment_size shadow local ones, fixed by renaming local variables
0006 - in xlogrecovery.c, some static file-scope variables shadow local variables, fixed by renaming local variables
0007 - cases of global progname shadows local, fixed by renaming local to myprogname
0008 - in file_ops.c, some static file-scope variables shadow local, fixed by renaming local variables
0009 - simple cases of local variables are shadowed by global, fixed by renaming local variables
0010 - a few more cases of static file-scope variables shadow local variables, fixed by renaming local variables
0011 - cases of global conn shadows local variables, fixed by renaming local conn to myconn
0012 - fixed shadowing in ecpg.header
0013 - fixed shadowing in all time-related modules. Heikki had a concern where there is a global named “days”, so there could be some discussions for this commit. For now, I just renamed local variables to avoid shadowing.
With this patch set, building postgres with “-Wshadow” won’t get any warning. Note, this patch set doesn’t cover contrib.
Best regards,