Re: Cleanup shadows variable warnings, round 1

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: Álvaro Herrera <alvherre(at)kurilemu(dot)de>
Cc: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Eisentraut <peter(at)eisentraut(dot)org>, Peter Smith <smithpb2250(at)gmail(dot)com>
Subject: Re: Cleanup shadows variable warnings, round 1
Date: 2026-04-21 07:01:31
Message-ID: 43421A9F-BA0D-4BB8-B9DB-61EB5C2D686F@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Mar 4, 2026, at 14:17, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
>
>
>> On Mar 3, 2026, at 18:26, Álvaro Herrera <alvherre(at)kurilemu(dot)de> wrote:
>>
>> 1. if you rename a function argument, then the function declaration
>> should match the new name as well.
>
> Fully addressed.
>
>> 2. xlogrecovery.c has far too many global variables. Can we use this
>> opportunity to try to get rid of some of them? Especially one called
>> "xlogreader" is I think quite bug-prone.
>
> I looked into this. There are quite a few file-scope static variables and global variables, and getting rid of them would likely require a fairly large refactoring.
>
> For now, I worked out an approach that wraps the file-scope static variables into a structure. I moved this change to the last commit and marked it as WIP. I plan to spend more time on the refactoring.
>
> In the meantime, I wonder if it would make sense to handle this refactoring in a separate patch.
>
>>
>> 3. I disagree with some of the choices made; for instance rather than
>> rename the local "progname" variables in all those places, I would
>> rename the global to logging_progname in logging.c;
>
> The progname conflicts are not caused by logging.c. Instead, it is declared in postmaster.h:
> ```
> extern PGDLLIMPORT const char *progname;
> ```
>
> I hesitate to rename this global since it is exported, and doing so might lead to additional changes elsewhere. For now, I moved this commit to the second last one, and I may spend more time investigating it.
>
>
>> in bringetbitmap
>> (0002) I would rename the outer "tmp" to "sizecheck" or something like
>> that. I guess this is mostly matter of mostly arbitrary judgment ...
>>
>
> I updated bringetbitmap to rename the outer variable.
>
> I also went through the whole patch and tuned a few other names. Please let me know if you disagree with any of the other renamings.
>
> PFA v7. Each commit is independent, so they do not need to be pushed in the same order as in this patch.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
> <v7-0001-cleanup-rename-inner-variables-to-avoid-shadowing.patch><v7-0002-cleanup-rename-outer-variables-to-avoid-shadowing.patch><v7-0003-cleanup-fix-macro-induced-variable-shadowing-in-i.patch><v7-0004-cleanup-avoid-local-wal_level-and-wal_segment_siz.patch><v7-0005-cleanup-avoid-local-variables-shadowed-by-static-.patch><v7-0006-cleanup-avoid-local-variables-shadowed-by-globals.patch><v7-0007-cleanup-avoid-local-variables-shadowed-by-static-.patch><v7-0008-cleanup-rename-local-conn-variables-to-avoid-shad.patch><v7-0009-cleanup-avoid-local-variables-shadowed-by-globals.patch><v7-0010-cleanup-avoid-local-variables-shadowed-by-globals.patch><v7-0011-cleanup-rename-local-progname-variables-to-avoid-.patch><v7-0012-WIP-xlogrecovery-consolidate-file-local-mutable-s.patch>

PFA v8 - rebased and fixed a few new occurrences.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachment Content-Type Size
v8-0001-cleanup-rename-inner-variables-to-avoid-shadowing.patch application/octet-stream 39.7 KB
v8-0002-cleanup-rename-outer-variables-to-avoid-shadowing.patch application/octet-stream 11.2 KB
v8-0003-cleanup-fix-macro-induced-variable-shadowing-in-i.patch application/octet-stream 6.1 KB
v8-0004-cleanup-avoid-local-wal_level-and-wal_segment_siz.patch application/octet-stream 3.5 KB
v8-0005-cleanup-avoid-local-variables-shadowed-by-static-.patch application/octet-stream 4.0 KB
v8-0006-cleanup-avoid-local-variables-shadowed-by-globals.patch application/octet-stream 9.5 KB
v8-0007-cleanup-avoid-local-variables-shadowed-by-static-.patch application/octet-stream 8.6 KB
v8-0008-cleanup-rename-local-conn-variables-to-avoid-shad.patch application/octet-stream 32.6 KB
v8-0009-cleanup-avoid-local-variables-shadowed-by-globals.patch application/octet-stream 2.5 KB
v8-0010-cleanup-avoid-local-variables-shadowed-by-globals.patch application/octet-stream 21.9 KB
v8-0011-cleanup-rename-local-progname-variables-to-avoid-.patch application/octet-stream 25.4 KB
v8-0012-WIP-xlogrecovery-consolidate-file-local-mutable-s.patch application/octet-stream 29.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Yuchen Li 2026-04-21 07:03:39 Re: repack: fix a bug to reject deferrable primary key fallback for concurrent mode
Previous Message Ashutosh Bapat 2026-04-21 06:58:08 Re: [Bug][patch]: After dropping the last label from a property graph element, invoking pg_get_propgraphdef() triggers an assertion failure