Subject: Remove shadowed declaration warnings

From: Peter Smith <[email-masked]>
Date: Thu, 12 Sep 2024 10:32:50 +1000

Hi,

I normally build the code with warnings enabled (specifically,
-Wshadow) which exposes many "shadowed" declarations.

It would be better to reduce warnings wherever it's easy to do so,
because if we always see/ignore lots of warnings then sooner or later
something important may escape attention. In any case, just removing
these warnings sometimes makes the code more readable by removing any
ambiguity.

Currently, I'm seeing 350+ shadow warnings. See the attached summary.

It will be WIP to eliminate them all, but here is a patch to address
some of the low-hanging fruit. If the patch is accepted, then I can
try later to deal with more of them.

======

The following are fixed by changing the param/var from 'text' to 'txt'.

execute.c:111:19: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]

~

prepare.c:104:26: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]
prepare.c:270:12: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]

~

c_keywords.c:36:32: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]
ecpg_keywords.c:39:35: warning: declaration of ‘text’ shadows a global
declaration [-Wshadow]
../../../../src/include/c.h:700:24: warning: shadowed declaration is
here [-Wshadow]

~

tab-complete.c:1638:29: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5082:46: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5111:38: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5120:36: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5129:37: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5140:33: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5148:43: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5164:40: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5172:50: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5234:19: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5605:32: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5685:33: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5733:37: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5778:33: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]
tab-complete.c:5910:27: warning: declaration of ‘text’ shadows a
global declaration [-Wshadow]
../../../src/include/c.h:700:24: warning: shadowed declaration is here
[-Wshadow]

======

The following was fixed by changing the name of the global static from
'progname' to 'prog_name'.

pg_createsubscriber.c:341:46: warning: declaration of ‘progname’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:114:20: warning: shadowed declaration is here [-Wshadow]

~

The following was fixed by changing the name of the global static from
'dbinfo' to 'db_info'.

pg_createsubscriber.c:437:25: warning: declaration of ‘dbinfo’ shadows
a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:734:40: warning: declaration of ‘dbinfo’ shadows
a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:841:46: warning: declaration of ‘dbinfo’ shadows
a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:961:47: warning: declaration of ‘dbinfo’ shadows
a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1104:41: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1142:41: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1182:45: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1242:54: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1272:56: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1314:70: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1363:60: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1553:57: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1627:55: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1681:64: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1739:69: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
pg_createsubscriber.c:1830:64: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: David Rowley <[email-masked]>
Date: Thu, 12 Sep 2024 12:58:19 +1200

> these warnings sometimes makes the code more readable by removing any

0fe954c28 did add -Wshadow=compatible-local to the standard set of
complication flags.  I felt it was diminishing returns after that, but
-Wshadow=local would be the next step before going full -Wshadow.

There was justification for -Wshadow=compatible-local because there
has been > 1 bug (see af7d270dd) fixed that would have been found if
we'd had that sooner. Have we ever had any bugs that would have been
highlighted by -Wshadow but not -Wshadow=compatible-local? I'd be
curious to know if you do go through this process of weeding these out
if you do find any bugs as a result.

I also wonder if we could ever get this to a stable point. I didn't
take the time to understand what 388e80132 did. Is that going to
protect us from getting warnings where fixing them is beyond our
control for full -Wshadow?

David

---

From: Tom Lane <[email-masked]>
Date: Wed, 11 Sep 2024 22:02:59 -0400

> -Wshadow=local would be the next step before going full -Wshadow.

I think that insisting that local declarations not shadow globals
is an anti-pattern, and I'll vote against any proposal to make
that a standard warning.  Impoverished as C is, it does have block
structure; why would we want to throw that away by (in effect)
demanding a single flat namespace for the entire program?

I do grant that sometimes shadowing of locals can cause bugs.  I don't
recall right now why we opted for -Wshadow=compatible-local over
-Wshadow=local, but we could certainly take another look at that.

			regards, tom lane

---

From: David Rowley <[email-masked]>
Date: Thu, 12 Sep 2024 14:25:38 +1200

> -Wshadow=local, but we could certainly take another look at that.

I don't recall if it was discussed, but certainly, it was an easier
goal to achieve.

Looks like there are currently 47 warnings with -Wshadow=local. I'm
not quite sure what the definition of "compatible" is for this flag,
but looking at one warning in pgbench.c:4548, I see an int shadowing a
bool. So maybe -Wshadow=local is worthwhile.

David

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 18 Sep 2024 09:53:11 +0200

> bool. So maybe -Wshadow=local is worthwhile.

Another thing to keep in mind with these different shadow warning 
variants is that we should try to keep them consistent across compilers, 
at least the common ones.  The current -Wshadow=compatible-local is only 
available in gcc, not in clang, so it's currently impossible to rely on 
clang to write warning-free code.

Of course we have other warning flags that we use that don't exist in 
all compilers, but in my experience these are either for very esoteric 
cases or something that is very obviously wrong and a developer would 
normally see immediately.  For example, there is no warning flag in 
clang that mirrors the switch "fallthrough" labeling that we have set up 
with gcc.  But this is not as much of a problem in practice because the 
wrong code would usually misbehave in an obvious way and the issue can 
be found by looking at the code with two lines of context.  With the 
shadow warning, the issues are much harder to find visually, and in most 
cases they are not actually a problem.

The shadow warning levels in gcc and clang appear to be very differently 
structured, so I'm not sure how we can improve interoperability here.