Message ID | VI1PR03MB45282E167E7A37DCBA2644E7E49F0@VI1PR03MB4528.eurprd03.prod.outlook.com |
---|---|
State | New |
Headers | show |
Series | Update documentation of -Wshadow | expand |
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? > > > Thanks > Bernd. > > > patch-wshadow-doc-invoke.diff > > 2019-10-03 Bernd Edlinger <bernd.edlinger@hotmail.de> > > * doc/invoke.texi (-Wshadow=global, -Wshadow=local, > -Wshadow=compatible-local): Fix description. > Add an example where -Wshadow=compatible-local does not > warn. OK jeff
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. If the code itself has changed so that -Wshadow=global no longer implies -Wshadow=local, then the added sentence is simply redundant as above. > @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
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
2019-10-03 Bernd Edlinger <bernd.edlinger@hotmail.de> * doc/invoke.texi (-Wshadow=global, -Wshadow=local, -Wshadow=compatible-local): Fix description. Add an example where -Wshadow=compatible-local does not warn. 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}. @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}. @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}. @item -Wlarger-than=@var{byte-size} @opindex Wlarger-than=