On 2025-Dec-03, Chao Li wrote:
> Unfortunately that doesn’t work for my compiler (clang on MacOS),
> that’s why I renamed “I" to “u”.
I think you're missing his point. He's suggesting to change not only
this variable, but also the other uses of "i" in this function.
I'd also change "unsigned" to "unsigned int", just because I dislike
using unadorned "unsigned" as a type name (I know it's a legal type
name.) This could be left alone, I guess -- not important.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
I misunderstood Peter's message yesterday. I have addressed both comments (changing all "for" and changing "unsigned" to "unsigned int") in v4.
On 2025-12-03 10:28:36 +0800, Chao Li wrote:
> 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.
This seems like a *lot* of noise / backpatching pain for relatively little
gain.
I agree the patch set is large, which is why I split it into smaller commits, each independent and self-contained.
The motivation is that CF’s CI currently fails on shadow-variable warnings. If you touch a file like a.c, and that file already has a legacy shadowing issue, CI will still fail your patch even if your changes are correct. Then you’re forced to fix unrelated shadow-variable problems just to get a clean CI run. I’ve run into this myself, and it’s disruptive for both patch authors and reviewers.
By cleaning up all existing shadow-variable warnings now, they won’t interfere with future patches, and it also allows us to enable -Wshadow by default so new shadowing issues won’t be introduced.
> /*
> - * Find a string representation for wal_level
> + * Find a string representation for wallevel
> */
> static const char *
> -get_wal_level_string(int wal_level)
> +get_wal_level_string(int wallevel)
> {
> const struct config_enum_entry *entry;
> - const char *wal_level_str = "?";
> + const char *wallevel_str = "?";
This sounds like it's talking about walls not, the WAL.
Fixed in v4.
V4 addressed all comments received so far, and fixed a mistake that caused CI failure.
Best regards,
Chao Li (Evan)
---------------------