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
