[PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Sat, Sep 10, 2016 at 09:51:57AM -0400, Eric Gallager wrote:
> On 9/10/16, Ian Lance Taylor wrote:
> > I'm not sure about the patch to configure.ac/configure. The last I
> > looked -Wshadow would warn if a local variable shadows a global
> > variable. That can cause a pointless build break if some system
> > header file defines a global variable that happens to have the same
> > name as a local variable. It's not a likely scenario but I don't see
> > a need to court a build breakage.
>
> Maybe if the patch to add -Wshadow-local went in, the configure script
> could use that instead? For reference:
> https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01317.html
That is a nice idea. Thanks. Lets try to get this in, I forward ported
it and cleaned things up a bit.
This patch from Le-Chun Wu adds two two new shadow warning flags for
C and C++:
-Wshadow-local which warns if a local variable shadows another local
variable or parameter,
-Wshadow-compatible-local which warns if a local variable shadows
another local variable or parameter whose type is compatible with that
of the shadowing variable.
It is already on the google/main branch (Google ref 39127) and was
previously submitted by Diego Novillo and reviewed on
http://codereview.appspot.com/4452058
I addressed the review comments and made the following changes:
- The Wshadow, Wshadow-local and -Wshadow-compatible-local relationships
are expressed in common.opt instead of in opts.c and documented in
invoke.texi.
- The "previous declaration" warnings were turned into notes and use
the (now) existing infrastructure instead of duplicating the warnings.
The testcases have been adjusted to expect the notes.
- The conditional change in name-lookup.c for non-locals (where we
don't want to change the warnings, but just check the global ones)
has been dropped.
gcc/ChangeLog:
2016-09-11 Le-Chun Wu
Mark Wielaard
* common.opt (Wshadow-local): New option.
(Wshadow-compatible-local): New option.
* doc/invoke.texi: Document Wshadow-local and Wshadow-compatible-local.
gcc/c/ChangeLog:
2016-09-11 Le-Chun Wu
Mark Wielaard
* c-decl.c (warn_if_shadowing): Use the warning code corresponding
to the given -Wshadow- variant.
gcc/cp/ChangeLog:
2016-09-11 Le-Chun Wu
Mark Wielaard
* name-lookup.c (pushdecl_maybe_friend): When emitting a
shadowing warning, use the code corresponding to the
given -Wshadow- variant.
---
gcc/c/c-decl.c | 39 +++---
gcc/common.opt | 10 +++-
gcc/cp/name-lookup.c | 28 --
gcc/doc/invoke.texi| 41 ++
.../g++.dg/warn/Wshadow-compatible-local-1.C | 63 ++
gcc/testsuite/g++.dg/warn/Wshadow-local-1.C| 35
gcc/testsuite/g++.dg/warn/Wshadow-local-2.C| 63 ++
gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c | 36 +
gcc/testsuite/gcc.dg/Wshadow-local-1.c | 22
gcc/testsuite/gcc.dg/Wshadow-local-2.c | 49 +
gcc/testsuite/gcc.dg/Wshadow-local-3.c | 9
14 files changed, 404 insertions(+), 11 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C
create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-local-1.C
create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-local-2.C
create mode 100644 gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c
create mode 100644 gcc/testsuite/gcc.dg/Wshadow-local-1.c
create mode 100644 gcc/testsuite/gcc.dg/Wshadow-local-2.c
create mode 100644 gcc/testsuite/gcc.dg/Wshadow-local-3.c
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 8f49c35..31f83d8 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -2735,7 +2735,9 @@ warn_if_shadowing (tree new_decl)
struct c_binding *b;
/* Shadow warnings wanted? */
- if (!warn_shadow
+ if (!(warn_shadow
+|| warn_shadow_local
+|| warn_shadow_compatible_local)
/* No shadow warnings for internally generated vars. */
|| DECL_IS_BUILTIN (new_decl)
/* No shadow warnings for vars made for inlining. */
@@ -2759,9 +2761,21 @@ warn_if_shadowing (tree new_decl)
break;
}
else if (TREE_CODE (old_decl) == PARM_DECL)
- warned = warning (OPT_Wshadow,
- "declaration of %q+D shadows a parameter",
- new_decl);
+ {
+ enum opt_code warning_code;
+
+ /* If '-Wshadow-compatible-local' is specified without other
+ -Wshadow flags, we will warn only when the types of the
+ shadowing variable (i.e. new_decl) and the shadowed variable
+ (old_decl) are compatible. */
+ if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl)))
+
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On 11/09/16 14:02, Mark Wielaard wrote:
-Wshadow-local which warns if a local variable shadows another local
variable or parameter,
-Wshadow-compatible-local which warns if a local variable shadows
another local variable or parameter whose type is compatible with that
of the shadowing variable.
I honestly don't see the need for the second flag. Why not make Wshadow, or at
least Wshadow-local, work in this way by default? Warning for shadowed
variables that will nevertheless trigger errors/warnings if used wrongly seems
not very useful anyway.
+ /* If '-Wshadow-compatible-local' is specified without other
+ -Wshadow flags, we will warn only when the types of the
+ shadowing variable (i.e. new_decl) and the shadowed variable
+ (old_decl) are compatible. */
+ if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl)))
+ warning_code = OPT_Wshadow_compatible_local;
+ else
+ warning_code = OPT_Wshadow_local;
+ warned = warning (warning_code,
+ "declaration of %q+D shadows a parameter",
+ new_decl);
Please don't use +D. Use warning_at with DECL_SOURCE_LOCATION(new_decl).
See: https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Locations
+ /* If '-Wshadow-compatible-local' is specified without other
+-Wshadow flags, we will warn only when the type of the
+shadowing variable (i.e. x) can be converted to that of
+the shadowed parameter (oldlocal). The reason why we only
+check if x's type can be converted to oldlocal's type
+(but not the other way around) is because when users
+accidentally shadow a parameter, more than often they
+would use the variable thinking (mistakenly) it's still
+the parameter. It would be rare that users would use the
+variable in the place that expects the parameter but
+thinking it's a new decl. */
As said above, IMHO, this behavior should be the default of -Wshadow, or at
least, of -Wshadow-local. The current behavior only leads to people not using
-Wshadow (and us not including it in -Wall -Wextra). There is a Linus rant from
some years ago that explains vehemently why Wshadow is useless in its current form.
+@item -Wshadow-compatible-local
+@opindex Wshadow-compatible-local
+@opindex Wno-shadow-compatible-local
+Warn when a local variable shadows another local variable or parameter
+whose type is compatible with that of the shadowing variable. In C++,
+type compatibility here means the type of the shadowing variable can be
+converted to that of the shadowed variable. The creation of this flag
+(in addition to @option{-Wshadow-local}) is based on the idea that when
+a local variable shadows another one of incompatible type, it is most
+likely intentional, not a bug or typo, as shown in the following example:
-Wshadow-compatible-local seems safe enough to be enabled by -Wall (or
-Wextra). Options not enabled by either are rarely used (and, hence, rarely
tested).
Cheers,
Manuel.
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
-Wextra, but it would be good to see some experiments using them first on some larger codebases (maybe gcc itself). If you could try it on some and report what the rate of false positive is (plus maybe some examples to show whether one might or might not want to fix them anyway) then we can see if it makes sense. But I believe that should be done independent from introducing the new warnings themselves. Thanks, Mark Attached v2 of the patch with the suggested change above using warning_at. No changes in the test results for any of the old or new -Wshadow tests. >From 909c48dc7abe77aefad30a8aac9177e18b5a188b Mon Sep 17 00:00:00 2001 From: Mark Wielaard Date: Sun, 11 Sep 2016 14:20:33 +0200 Subject: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local. This patch from Le-Chun Wu adds two new shadow warning flags for C and C++: -Wshadow-local which warns if a local variable shadows another local variable or parameter, -Wshadow-compatible-local which warns if a local variable shadows another local variable or parameter whose type is compatible with that of the shadowing variable. It is already on the google/main branch (Google ref 39127) and was previously submitted by Diego Novillo and reviewed on http://codereview.appspot.com/4452058 I addressed the review comments and made the following changes: - The -Wshadow, -Wshadow-local and -Wshadow-compatible-local relationships are expressed in common.opt instead of in opts.c and documented in invoke.texi. - The "previous declaration" warnings were turned into notes and use the (now) existing infrastructure instead of duplicating the warnings. The testcases have been adjusted to expect the notes. - The conditional change in name-lookup.c for non-locals (where we don't want to change the warnings, but just check the global ones) has been dropped. v2 - Use warning_at in c-decl.c (warn_if_shadowing). gcc/ChangeLog: 2016-09-11 Le-Chun Wu Mark Wielaard * common.opt (Wshadow-local): New option. (Wshadow-compatible-local): New option. * doc/invoke.texi: Document Wshadow-local and Wshadow-compatible-local. gcc/c/ChangeLog: 2016-09-11 Le-Chun Wu Mark Wielaard * c-decl.c (warn_if_shadowing): Use the warning code corresponding to the given -Wshadow- variant. gcc/cp/ChangeLog: 2016-09-11 Le-Chun Wu Mark Wielaard * name-lookup.c (pushdecl_maybe_friend): When emitting a shadowing warning, use the code corresponding to the given -Wshadow- variant. --- gcc/ChangeLog | 7 +++ gcc/c/ChangeLog| 6 +++ gcc/c/c-decl.c | 39 +++--- gcc/common.opt | 10 +++- gcc/cp/ChangeLog | 7 +++ gcc/cp/name-lookup.c | 28 -- gcc/doc/invoke.texi| 41 ++ .../g++.dg/warn/Wshadow-compatible-local-1.C | 63 ++ gcc/testsuite/g++.dg/warn/Wshadow-local-1.C| 35 gcc/testsuite/g++.dg/warn/Wshadow-local-2.C| 63 ++ gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c | 36 + gcc/testsuite/gcc.dg/Wshadow-local-1.c | 22 gcc/testsuite/gcc.dg/Wshadow-local-2.c | 49 + gcc/testsuite/gcc.dg/Wshadow-local-3.c | 9 14 files changed, 404 insertions(+), 11 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-local-1.C create mode 100644 gcc/testsuite/g++.dg/warn/Wshadow-local-2.C create mode 100644 gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c create mode 100644 gcc/testsuite/gcc.dg/Wshadow-local-1.c create mode 100644 gcc/testsuite/gcc.dg/Wshadow-local-2.c create mode 100644 gcc/testsuite/gcc.dg/Wshadow-local-3.c diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 2219333..c4486dc 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2016-09-11 Le-Chun Wu + Mark Wielaard + + * common.opt (Wshadow-local): New option. + (Wshadow-compatible-local): New option. + * doc/invoke.texi: Document Wshadow-local and Wshadow-compatible-local. + 2016-09-09 Peter Bergner PR rtl-optimization/77289 diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog index a647263..23ee5f1 100644 --- a/gcc/c/ChangeLog +++ b/gcc/c/ChangeLog @@ -1,3 +1,9 @@ +2016-09-11 Le-Chun Wu + Mark Wielaard + + * c-decl.c (warn_if_shadowing): Use the warning code corresponding + to the given -Wshadow- variant. + 2016-09-07 David Malcolm * c-lang.c (LANG_HOOKS_GET_SUBSTRING_LOCATION): Use diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 8f49c35..6a79781 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On 9/11/16, Manuel López-Ibáñez wrote:
> On 11/09/16 14:02, Mark Wielaard wrote:
>> -Wshadow-local which warns if a local variable shadows another local
>> variable or parameter,
>>
>> -Wshadow-compatible-local which warns if a local variable shadows
>> another local variable or parameter whose type is compatible with that
>> of the shadowing variable.
>
> I honestly don't see the need for the second flag. Why not make Wshadow, or at
> least Wshadow-local, work in this way by default? Warning for shadowed
> variables that will nevertheless trigger errors/warnings if used wrongly
> seems not very useful anyway.
>
>
It helps for readability and helps pre-emptively catch those other
errors/warnings that come from using them wrongly before they occur in
a more confusing manner. Plus more granularity makes it easier to
manage the workload of warning-silencing.
>> +/* If '-Wshadow-compatible-local' is specified without other
>> + -Wshadow flags, we will warn only when the types of the
>> + shadowing variable (i.e. new_decl) and the shadowed variable
>> + (old_decl) are compatible. */
>> +if (comptypes (TREE_TYPE (old_decl), TREE_TYPE (new_decl)))
>> + warning_code = OPT_Wshadow_compatible_local;
>> +else
>> + warning_code = OPT_Wshadow_local;
>> +warned = warning (warning_code,
>> + "declaration of %q+D shadows a parameter",
>> + new_decl);
>
> Please don't use +D. Use warning_at with DECL_SOURCE_LOCATION(new_decl).
> See: https://gcc.gnu.org/wiki/DiagnosticsGuidelines#Locations
>
>
>> + /* If '-Wshadow-compatible-local' is specified without other
>> + -Wshadow flags, we will warn only when the type of the
>> + shadowing variable (i.e. x) can be converted to that of
>> + the shadowed parameter (oldlocal). The reason why we only
>> + check if x's type can be converted to oldlocal's type
>> + (but not the other way around) is because when users
>> + accidentally shadow a parameter, more than often they
>> + would use the variable thinking (mistakenly) it's still
>> + the parameter. It would be rare that users would use the
>> + variable in the place that expects the parameter but
>> + thinking it's a new decl. */
>
> As said above, IMHO, this behavior should be the default of -Wshadow, or at
> least, of -Wshadow-local. The current behavior only leads to people not
> using -Wshadow (and us not including it in -Wall -Wextra). There is a Linus
> rant
> from some years ago that explains vehemently why Wshadow is useless in its
> current form.
>
>> +@item -Wshadow-compatible-local
>> +@opindex Wshadow-compatible-local
>> +@opindex Wno-shadow-compatible-local
>> +Warn when a local variable shadows another local variable or parameter
>> +whose type is compatible with that of the shadowing variable. In C++,
>> +type compatibility here means the type of the shadowing variable can be
>> +converted to that of the shadowed variable. The creation of this flag
>> +(in addition to @option{-Wshadow-local}) is based on the idea that when
>> +a local variable shadows another one of incompatible type, it is most
>> +likely intentional, not a bug or typo, as shown in the following
>> example:
>
> -Wshadow-compatible-local seems safe enough to be enabled by -Wall (or
> -Wextra). Options not enabled by either are rarely used (and, hence, rarely
> tested).
>
> Cheers,
>
> Manuel.
>
I see lots of GNU projects at least using the gnulib manywarnings.m4
autoconf macros in their configure script, and that enables a lot more
warnings than just -Wall and -Wextra. So I think the test coverage for
warnings outside of -Wall or -Wextra is better than you might think.
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager wrote: > On 9/11/16, Manuel López-Ibáñez wrote: >> On 11/09/16 14:02, Mark Wielaard wrote: >>> -Wshadow-local which warns if a local variable shadows another local >>> variable or parameter, >>> >>> -Wshadow-compatible-local which warns if a local variable shadows >>> another local variable or parameter whose type is compatible with that >>> of the shadowing variable. Hi Mark, Thank you for doing this! >> I honestly don't see the need for the second flag. Why not make Wshadow, or >> at >> least Wshadow-local, work in this way by default? Warning for shadowed >> variables that will nevertheless trigger errors/warnings if used wrongly >> seems not very useful anyway. > > It helps for readability and helps pre-emptively catch those other > errors/warnings that come from using them wrongly before they occur in > a more confusing manner. Plus more granularity makes it easier to > manage the workload of warning-silencing. The new warnings will be especially welcome in C++ code. -Wshadow is fine for most C code, but in C++ it is very common to have method names that shadow class data member names and/or local variables in any member function definition. Thus, using -Wshadow in C++ code often forces undesirable (and unscalable) renaming to avoid such shadowing, while the only important type of shadowing is that caught by the new options. Many of us want the benefit of the new options (spotting bad, easily-fixed code), without having to incur the renaming (often hurting readability/maintainability) required to enable -Werror=shadow.
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Mon, Sep 12, 2016 at 1:38 PM, Jim Meyering wrote:
> On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager wrote:
>> On 9/11/16, Manuel López-Ibáñez wrote:
>>> On 11/09/16 14:02, Mark Wielaard wrote:
-Wshadow-local which warns if a local variable shadows another local
variable or parameter,
-Wshadow-compatible-local which warns if a local variable shadows
another local variable or parameter whose type is compatible with that
of the shadowing variable.
>
> Hi Mark,
> Thank you for doing this!
>
>>> I honestly don't see the need for the second flag. Why not make Wshadow, or
>>> at
>>> least Wshadow-local, work in this way by default? Warning for shadowed
>>> variables that will nevertheless trigger errors/warnings if used wrongly
>>> seems not very useful anyway.
>>
>> It helps for readability and helps pre-emptively catch those other
>> errors/warnings that come from using them wrongly before they occur in
>> a more confusing manner. Plus more granularity makes it easier to
>> manage the workload of warning-silencing.
>
> The new warnings will be especially welcome in C++ code.
> -Wshadow is fine for most C code, but in C++ it is very common to have
> method names that shadow class data member names and/or local
> variables in any member function definition. Thus, using -Wshadow in
> C++ code often forces undesirable (and unscalable) renaming to avoid
> such shadowing, while the only important type of shadowing is that
> caught by the new options. Many of us want the benefit of the new
> options (spotting bad, easily-fixed code), without having to incur the
> renaming (often hurting readability/maintainability) required to
> enable -Werror=shadow.
I wonder about spelling the options as
-Wshadow={local,compatible-local} rather than with a dash, but
otherwise the patch looks fine.
Jason
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote:
> I wonder about spelling the options as
> -Wshadow={local,compatible-local} rather than with a dash, but
> otherwise the patch looks fine.
That is a much nicer way to write the option. But if I do that I would
like to keep the old names as aliases because Google already ships a gcc
that accepts -Wshadow-local and -Wshadow-compatible-local and you can
find programs that already probe for those names in their configure
scripts. Can I make the existing names hidden aliases by marking them
Undocumented in the .opt file? Or is that too contrived/ugly?
Thanks,
Mark
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On 9/14/16, Jason Merrill wrote:
> On Mon, Sep 12, 2016 at 1:38 PM, Jim Meyering wrote:
>> On Mon, Sep 12, 2016 at 7:05 AM, Eric Gallager
>> wrote:
>>> On 9/11/16, Manuel López-Ibáñez wrote:
On 11/09/16 14:02, Mark Wielaard wrote:
> -Wshadow-local which warns if a local variable shadows another local
> variable or parameter,
>
> -Wshadow-compatible-local which warns if a local variable shadows
> another local variable or parameter whose type is compatible with
> that
> of the shadowing variable.
>>
>> Hi Mark,
>> Thank you for doing this!
>>
I honestly don't see the need for the second flag. Why not make Wshadow,
or at
least Wshadow-local, work in this way by default? Warning for shadowed
variables that will nevertheless trigger errors/warnings if used
wrongly
seems not very useful anyway.
>>>
>>> It helps for readability and helps pre-emptively catch those other
>>> errors/warnings that come from using them wrongly before they occur in
>>> a more confusing manner. Plus more granularity makes it easier to
>>> manage the workload of warning-silencing.
>>
>> The new warnings will be especially welcome in C++ code.
>> -Wshadow is fine for most C code, but in C++ it is very common to have
>> method names that shadow class data member names and/or local
>> variables in any member function definition. Thus, using -Wshadow in
>> C++ code often forces undesirable (and unscalable) renaming to avoid
>> such shadowing, while the only important type of shadowing is that
>> caught by the new options. Many of us want the benefit of the new
>> options (spotting bad, easily-fixed code), without having to incur the
>> renaming (often hurting readability/maintainability) required to
>> enable -Werror=shadow.
>
> I wonder about spelling the options as
> -Wshadow={local,compatible-local} rather than with a dash, but
> otherwise the patch looks fine.
>
> Jason
>
What would the current behavior be called? Maybe
-Wshadow={all,local,compatible-local} instead? Also remember -Werror:
with another '=' you could end up with 2 of them with something like
-Werror=shadow=local which looks more confusing than the hyphenated
version.
Eric
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Wed, Sep 14, 2016 at 1:30 AM, Mark Wielaard wrote:
> On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote:
>> I wonder about spelling the options as
>> -Wshadow={local,compatible-local} rather than with a dash, but
>> otherwise the patch looks fine.
>
> That is a much nicer way to write the option. But if I do that I would
> like to keep the old names as aliases because Google already ships a gcc
> that accepts -Wshadow-local and -Wshadow-compatible-local and you can
> find programs that already probe for those names in their configure
> scripts. Can I make the existing names hidden aliases by marking them
> Undocumented in the .opt file? Or is that too contrived/ugly?
I don't have any opinion as to what the option names should be, but I
don't see the fact that Google's GCC has different option names as a
concern. That GCC is only used within Google, and Google is moving to
LLVM in any case. Changing the option names in GCC trunk is not a
problem for anybody.
Ian
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Wed, 2016-09-14 at 05:39 -0700, Ian Lance Taylor wrote:
> On Wed, Sep 14, 2016 at 1:30 AM, Mark Wielaard wrote:
> > On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote:
> >> I wonder about spelling the options as
> >> -Wshadow={local,compatible-local} rather than with a dash, but
> >> otherwise the patch looks fine.
> >
> > That is a much nicer way to write the option. But if I do that I would
> > like to keep the old names as aliases because Google already ships a gcc
> > that accepts -Wshadow-local and -Wshadow-compatible-local and you can
> > find programs that already probe for those names in their configure
> > scripts. Can I make the existing names hidden aliases by marking them
> > Undocumented in the .opt file? Or is that too contrived/ugly?
>
> I don't have any opinion as to what the option names should be, but I
> don't see the fact that Google's GCC has different option names as a
> concern. That GCC is only used within Google
Google did release a gcc with those warning options (I believe as part
of the NDK) and there are various projects out there (firefox seems one
of them) that check to see if these options are available before
enabling/disabling them (I don't know if other compilers implemented
them). Given that there are public sources out there that already seem
to use/test for these option names I would prefer to keep the
compatibility.
Cheers,
Mark
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Wed, Sep 14, 2016 at 5:49 AM, Mark Wielaard wrote:
> On Wed, 2016-09-14 at 05:39 -0700, Ian Lance Taylor wrote:
>> On Wed, Sep 14, 2016 at 1:30 AM, Mark Wielaard wrote:
>> > On Wed, 2016-09-14 at 00:00 -0400, Jason Merrill wrote:
>> >> I wonder about spelling the options as
>> >> -Wshadow={local,compatible-local} rather than with a dash, but
>> >> otherwise the patch looks fine.
>> >
>> > That is a much nicer way to write the option. But if I do that I would
>> > like to keep the old names as aliases because Google already ships a gcc
>> > that accepts -Wshadow-local and -Wshadow-compatible-local and you can
>> > find programs that already probe for those names in their configure
>> > scripts. Can I make the existing names hidden aliases by marking them
>> > Undocumented in the .opt file? Or is that too contrived/ugly?
>>
>> I don't have any opinion as to what the option names should be, but I
>> don't see the fact that Google's GCC has different option names as a
>> concern. That GCC is only used within Google
>
> Google did release a gcc with those warning options (I believe as part
> of the NDK) and there are various projects out there (firefox seems one
> of them) that check to see if these options are available before
> enabling/disabling them (I don't know if other compilers implemented
> them). Given that there are public sources out there that already seem
> to use/test for these option names I would prefer to keep the
> compatibility.
Thanks again for working on this.
I have been using these new options (locally patched) to good effect.
While the vast majority of warning-triggering code has been
technically correct, using these has uncovered at least 4 or 5 real
bugs in code I care about.
I see that these new options are not yet on master. Is there anything
I can do to help get this patch accepted?
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Mon, Oct 24, 2016 at 04:26:49PM -0700, Jim Meyering wrote: > I have been using these new options (locally patched) to good effect. > While the vast majority of warning-triggering code has been > technically correct, using these has uncovered at least 4 or 5 real > bugs in code I care about. Awesome. Thanks for testing. > I see that these new options are not yet on master. Is there anything > I can do to help get this patch accepted? If you could get me 48 hour days that would help :) Sorry, I just ran out of time. I am just a spare time gcc hacker and somehow my spare time disappeared. I think the only thing "blocking" the patch from going in is that nobody made a decission on how the actual warning option should be named. I think the suggestion for -Wshadow=[global|local|compatible-local] is the right one. With -Wshadow being an alias for -Wshadow=global. But since there are already gcc versions out there that accept -Wshadow-local and -Wshadow-compatible-local (and you can find some configure scripts that already check for those options) it would be good to have those as (hidden) aliases. If people, some maintainer, agrees with that then we can do the .opt file hacking to make it so. Cheers, Mark
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
On Tue, Oct 25, 2016 at 01:57:09AM +0200, Mark Wielaard wrote: > I think the only thing "blocking" the patch from going in is that > nobody made a decission on how the actual warning option should be > named. I think the suggestion for -Wshadow=[global|local|compatible-local] > is the right one. With -Wshadow being an alias for -Wshadow=global. > But since there are already gcc versions out there that accept > -Wshadow-local and -Wshadow-compatible-local (and you can find some > configure scripts that already check for those options) it would be > good to have those as (hidden) aliases. > > If people, some maintainer, agrees with that then we can do the .opt > file hacking to make it so. Nobody objected, nor did anybody say this is a great idea. But I think it is. So I just implemented the options this way. I made one small diagnostic change to fix a regression pointed out by gcc/testsuite/gcc.dg/pr48062.c. It should still be possible to ignore all shadow warnings with #pragma GCC diagnostic ignored "-Wshadow" when -Wshadow was given, but not the new -Wshadow=(local|compatible-local). So we now set warning_code = OPT_Wshadow when -Wshadow was given. The documentation and code comments were updated to refer to the new -Wshadow=... variants. OK to commit the attached patch? Thanks, Mark >From 390697e924926d7f8e451d39114de4903db07325 Mon Sep 17 00:00:00 2001 From: Mark Wielaard Date: Sun, 11 Sep 2016 14:20:33 +0200 Subject: [PATCH] Add -Wshadow=global -Wshadow=local and -Wshadow=compatible-local. This patch from Le-Chun Wu adds two new shadow warning flags for C and C++: -Wshadow=local which warns if a local variable shadows another local variable or parameter, -Wshadow=compatible-local which warns if a local variable shadows another local variable or parameter whose type is compatible with that of the shadowing variable. It is already on the google/main branch (Google ref 39127) and was previously submitted by Diego Novillo and reviewed on http://codereview.appspot.com/4452058 I addressed the review comments and made the following changes: - Add -Wshadow=global (the default alias for -Wshadow). - Make the documented options -Wshadow=global, -Wshadow=local and -Wshadow=compatible-local (with hidden undocumented aliases -Wshadow-local and -Wshadow-compatible-local for compatibility). - The -Wshadow=global, -Wshadow=local and -Wshadow=compatible-local relationships are expressed in common.opt instead of in opts.c and documented in invoke.texi. - The "previous declaration" warnings were turned into notes and use the (now) existing infrastructure instead of duplicating the warnings. The testcases have been adjusted to expect the notes. - The conditional change in name-lookup.c for non-locals (where we don't want to change the warnings, but just check the global ones) has been dropped. - Use warning_at in c-decl.c (warn_if_shadowing). gcc/ChangeLog: 2016-10-30 Le-Chun Wu Mark Wielaard * doc/invoke.texi: Document Wshadow-local and Wshadow-compatible-local. * common.opt (Wshadow=global): New option. Default for -Wshadow. (Wshadow=local): New option. (Wshadow-local): Hidden alias for -Wshadow=local. (Wshadow=compatible-local): New option. (Wshadow-compatible-local): Hidden alias for -Wshadow=compatible-local. * doc/invoke.texi: Document Wshadow=global, Wshadow=local and Wshadow=compatible-local. gcc/c/ChangeLog: 2016-10-30 Le-Chun Wu Mark Wielaard * c-decl.c (warn_if_shadowing): Use the warning code corresponding to the given -Wshadow= variant. Use warning_at. gcc/cp/ChangeLog: 2016-10-30 Le-Chun Wu Mark Wielaard * name-lookup.c (pushdecl_maybe_friend): When emitting a shadowing warning, use the code corresponding to the given -Wshadow= variant. gcc/testsuite/ChangeLog 2016-10-30 Le-Chun Wu Mark Wielaard * gcc.dg/Wshadow-compatible-local-1.c: New test. * gcc.dg/Wshadow-local-1.c: Likewise. * gcc.dg/Wshadow-local-2.c: Likewise. * g++.dg/warn/Wshadow-compatible-local-1.C: Likewise. * g++.dg/warn/Wshadow-local-1.C: Likewise. * g++.dg/warn/Wshadow-local-2.C: Likewise. --- diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c index 136f304..3e1b7a4 100644 --- a/gcc/c/c-decl.c +++ b/gcc/c/c-decl.c @@ -2735,7 +2735,9 @@ warn_if_shadowing (tree new_decl) struct c_binding *b; /* Shadow warnings wanted? */ - if (!warn_shadow + if (!(warn_shadow +|| warn_shadow_local +|| warn_shadow_compatible_local) /* No shadow warnings for internally generated vars. */ || DECL_IS_BUILTIN (new_decl) /* No shadow warnings for vars made for inlining. */ @@ -2759,9 +2761,23 @@ warn_if_shadowing (tree new_decl) break; } else if (TREE_CODE (old_decl) == PARM_DECL) - warned = warning (OPT_Wshadow, -
Re: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
OK. On Sun, Oct 30, 2016 at 3:53 PM, Mark Wielaard wrote: > On Tue, Oct 25, 2016 at 01:57:09AM +0200, Mark Wielaard wrote: >> I think the only thing "blocking" the patch from going in is that >> nobody made a decission on how the actual warning option should be >> named. I think the suggestion for -Wshadow=[global|local|compatible-local] >> is the right one. With -Wshadow being an alias for -Wshadow=global. >> But since there are already gcc versions out there that accept >> -Wshadow-local and -Wshadow-compatible-local (and you can find some >> configure scripts that already check for those options) it would be >> good to have those as (hidden) aliases. >> >> If people, some maintainer, agrees with that then we can do the .opt >> file hacking to make it so. > > Nobody objected, nor did anybody say this is a great idea. But I think > it is. So I just implemented the options this way. > > I made one small diagnostic change to fix a regression pointed out by > gcc/testsuite/gcc.dg/pr48062.c. It should still be possible to ignore > all shadow warnings with #pragma GCC diagnostic ignored "-Wshadow" when > -Wshadow was given, but not the new -Wshadow=(local|compatible-local). > So we now set warning_code = OPT_Wshadow when -Wshadow was given. > > The documentation and code comments were updated to refer to the > new -Wshadow=... variants. > > OK to commit the attached patch? > > Thanks, > > Mark
