Re: Cleanup shadows variable warnings, round 1

On Wed, Dec 3, 2025 at 10:36 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:

> 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 Wed, Dec 3, 2025 at 11:17 PM Andres Freund <andres@anarazel.de> wrote:

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


On Wed, Dec 3, 2025 at 11:17 PM Andres Freund <andres@anarazel.de> wrote:

>
> >  /*
> > - * 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)
---------------------
HighGo Software Co., Ltd.
https://www.highgo.com/