Update documentation of -Wshadow

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.

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