Hi Sandra, On 10/6/19 10:23 PM, Sandra Loosemore wrote: > On 10/3/19 9:16 AM, Bernd Edlinger wrote: >> Hi, >> >> I've noticed that the documentation of -Wshadow=x has some >> missing bits, and I want to add an negative example to >> -Wshadow=compatble-local. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. >> Is it OK for trunk? > > >> Index: gcc/doc/invoke.texi >> =================================================================== >> --- gcc/doc/invoke.texi (revision 276484) >> +++ gcc/doc/invoke.texi (working copy) >> @@ -6477,13 +6477,14 @@ Do not warn whenever a local variable shadows an i >> Objective-C method. >> >> @item -Wshadow=global >> -@opindex Wshadow=local >> +@opindex Wshadow=global >> The default for @option{-Wshadow}. Warns for any (global) shadowing. >> +This warning is enabled by @option{-Wshadow=global}. > > This is redundant and adds nothing to the description except to make it more > verbose. > >> >> @item -Wshadow=local >> @opindex Wshadow=local >> Warn when a local variable shadows another local variable or parameter. >> -This warning is enabled by @option{-Wshadow=global}. >> +This warning is enabled by @option{-Wshadow=local}. > > I think this incorrectly changes the meaning. What the existing > documentation is saying is that -Wshadow=global *also* enables warnings about > local variables as if -Wshadow=local were specified explicitly. >
Ah, I seed, that is a rather deep enable cascade: -Wshadow => -Wshadow=global => -Wshadow=local => -Wshadow=compatible-local Maybe we should then say -Wshadow=compatible-local is also enabled by -Wshadow=local, -Wshadow=global and -Wshadow, so I do not have to read all the other -Wshadow=x options to figure out that this one is actually enabled by -Wshadow ? I think that is bit hard to understand, since usually we have that with -Wextra and -Wall, but they have depth 1. > If the code itself has changed so that -Wshadow=global no longer implies > -Wshadow=local, then the added sentence is simply redundant as above. > No the implementation did not change, I just did not understand what the documentation is trying to tell. I thought the value x after -Wshadow=x was a level. But the documentation does imply that -Wshadow is implicitly enables -Wshadow=global and -Wshadow=global is enabled by -Wshadow or -Wshadow=global and implicitly enables -Wshadow=local -Wshadow=local is enabled by -Wshadow or -Wshadow=global or -Wshadow=local and implicitly enables -Wshadow=compatible-local. This would imply that I can use -Wshadow=global -Wno-shadow=local to enable just the global or -Wshadow=local together with -Wshadow=compatible-local but as the following test case shows that is not the case: $ cat test.c int c; void foo(int *c, int *d) { int *e = d; { int d = 0; } { int *e = 0; } } $ gcc -Wshadow=global -Wno-shadow=local -c test.c test.c: In function 'foo': test.c:2:15: warning: declaration of 'c' shadows a global declaration [-Wshadow] 2 | void foo(int *c, int *d) | ~~~~~^ test.c:1:5: note: shadowed declaration is here 1 | int c; | ^ test.c:6:7: warning: declaration of 'd' shadows a parameter [-Wshadow] 6 | int d = 0; | ^ test.c:2:23: note: shadowed declaration is here 2 | void foo(int *c, int *d) | ~~~~~^ test.c:9:8: warning: declaration of 'e' shadows a previous local [-Wshadow] 9 | int *e = 0; | ^ test.c:4:7: note: shadowed declaration is here 4 | int *e = d; | ^ I think it works more like a level, but also not exactly, since $ gcc -Wshadow=local -c test.c test.c: In function 'foo': test.c:6:7: warning: declaration of 'd' shadows a parameter [-Wshadow=local] 6 | int d = 0; | ^ test.c:2:23: note: shadowed declaration is here 2 | void foo(int *c, int *d) | ~~~~~^ test.c:9:8: warning: declaration of 'e' shadows a previous local [-Wshadow=compatible-local] 9 | int *e = 0; | ^ test.c:4:7: note: shadowed declaration is here 4 | int *e = d; | ^ $ gcc -Wshadow=local -Wno-shadow=compatible-local -c test.c test.c: In function 'foo': test.c:6:7: warning: declaration of 'd' shadows a parameter [-Wshadow=local] 6 | int d = 0; | ^ test.c:2:23: note: shadowed declaration is here 2 | void foo(int *c, int *d) | ~~~~~^ $ gcc -Wshadow=compatible-local -c test.c test.c: In function 'foo': test.c:9:8: warning: declaration of 'e' shadows a previous local [-Wshadow=compatible-local] 9 | int *e = 0; | ^ test.c:4:7: note: shadowed declaration is here 4 | int *e = d; | ^ should change the level from local to compatible-local, but here they behave like independent options. I actually expected them to behave like a warning-level option, like -Wimplicit-fallthrough=<0,5>, and was therefore confused by the doc. But actually the impementation seems to be a mix of both concepts... Bernd. >> @item -Wshadow=compatible-local >> @opindex Wshadow=compatible-local >> @@ -6515,8 +6516,10 @@ in place of the other, type checking will catch th >> warning. So not warning (about shadowing) in this case will not lead to >> undetected bugs. Use of this flag instead of @option{-Wshadow=local} can >> possibly reduce the number of warnings triggered by intentional shadowing. >> +Note that this does also mean that shadowing @code{const char *i} by >> +@code{char *i} will not emit a warning. >> >> -This warning is enabled by @option{-Wshadow=local}. >> +This warning is enabled by @option{-Wshadow=compatible-local}. > > Similar issues here; the existing docs are saying -Wshadow=local implies > -Wshadow=compatible-local. > >> >> @item -Wlarger-than=@var{byte-size} >> @opindex Wlarger-than= > > If you want to try again to fix the documentation, please also fix this > > -Wshadow=global, -Wshadow=local, -Wshadow=compatible-local @gol > > in the "Option Summary" (we don't use commas as delimiters in these tables) > and please try to rewrite the paragraph discussing the existing > -Wshadow=compatible-local example to s/variable/variables/ and use the > present tense instead of the future: > > Since the two variable @code{i} in the example above have incompatible types, > enabling only @option{-Wshadow=compatible-local} will not emit a warning. > Because their types are incompatible, if a programmer accidentally uses one > in place of the other, type checking will catch that and emit an error or > warning. So not warning (about shadowing) in this case will not lead to > undetected bugs. Use of this flag instead of @option{-Wshadow=local} can > possibly reduce the number of warnings triggered by intentional shadowing. > > -Sandra