MIME-Version: 1.0
References: <aeeADDomU6yAjCOb@alvherre.pgsql>
 <8B830174-7794-411F-B936-37DDB509EC5D@gmail.com>
In-Reply-To: <8B830174-7794-411F-B936-37DDB509EC5D@gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Thu, 23 Apr 2026 17:19:22 +1200
Message-ID: 
 <CAApHDvp1AnEsoqi7jHAn-p9ejqwYx=-21Zw7d2HrmSU2vry7Hg@mail.gmail.com>
Subject: Re: Cleanup shadows variable warnings, round 1
To: Chao Li <li.evan.chao@gmail.com>
Cc: =?UTF-8?Q?=C3=81lvaro_Herrera?= <alvherre@kurilemu.de>,
	Postgres hackers <pgsql-hackers@lists.postgresql.org>,
	Peter Eisentraut <peter@eisentraut.org>, Peter Smith <smithpb2250@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvp1AnEsoqi7jHAn-p9ejqwYx%3D-21Zw7d2HrmSU2vry7Hg%40mail.gmail.com>
Precedence: bulk

On Wed, 22 Apr 2026 at 17:14, Chao Li <li.evan.chao@gmail.com> wrote:
> The attached new v1 patch fixes the v19-only shadow warnings. There are not many. I strictly limited it to warnings newly introduced in v19, without touching any pre-existing ones, even where an old occurrence is very close to a new one.

Thank you. I pushed those after some adjustments.

While doing that, I did think more on if we should do more of this for
v20. I keep thinking back to the times when I've had to write 6
different versions of a patch to back patch to 6 different branches.
It's rarely that bad, but it sure does make you swear when the 6th
"git am" fails, especially when you find out that it was for a very
trivial thing, such as a spelling mistake fix. You really have to
fight off the temptation of complacency after the first 3 or so failed
git ams.

A worse category of problems that this particular set of patches could
cause is no conflict when we want one. I personally always write bug
fixes for master and back-patch them, but if anyone were to work
forward to newer versions, then imagine someone adding some code to a
function that does something with a local variable that's shadowed
globally. If they forward patch that to a version where the local
variable has been renamed, everything compiles and might appear to
work, but it's now the global that's being changed when the new code
was meant to change the local. Maybe no committers work that way, but
if they do, it's a real risk.

IMO, without any references to recent bugs that have been fixed due to
shadowing, then I can't see beyond the fact that this might be more
likely to cause bugs than to prevent them. As I recall, we were about
borderline on doing -Wshadow=compatible-local.  At least for
non-compatible variables, I'd expect you'd get a warning or error
during compilation. For the record, I got motivated for Justin's work
on the compatible-local due to af7d270dd. I removed a shadowed
variable which was incorrect. In my view, Justin Pryzby's proposal to
do something about this was well timed. I'm not seeing the same thing
happen here. Maybe I missed it?

David