Remove shadowed declaration warnings
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
On Thu, 12 Sept 2024 at 12:33, Peter Smith <smithpb2250@gmail.com> wrote:
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.
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
David Rowley <dgrowleyml@gmail.com> writes:
On Thu, 12 Sept 2024 at 12:33, Peter Smith <smithpb2250@gmail.com> wrote:
I normally build the code with warnings enabled (specifically,
-Wshadow) which exposes many "shadowed" declarations.
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.
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
On Thu, 12 Sept 2024 at 14:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.
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
On 12.09.24 04:25, David Rowley wrote:
On Thu, 12 Sept 2024 at 14:03, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.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.
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.