On Sat, Sep 10, 2016 at 09:51:57AM -0400, Eric Gallager wrote: > On 9/10/16, Ian Lance Taylor <i...@google.com> 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 <l...@google.com> Mark Wielaard <m...@redhat.com> * 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 <l...@google.com> Mark Wielaard <m...@redhat.com> * 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 <l...@google.com> Mark Wielaard <m...@redhat.com> * 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))) + 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); + } else if (DECL_FILE_SCOPE_P (old_decl)) { /* Do not warn if a variable shadows a function, unless @@ -2784,8 +2798,21 @@ warn_if_shadowing (tree new_decl) break; } else - warned = warning (OPT_Wshadow, "declaration of %q+D shadows a " - "previous local", 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))) + warning_code = OPT_Wshadow_compatible_local; + else + warning_code = OPT_Wshadow_local; + warned = warning (warning_code, + "declaration of %q+D shadows a previous local", + new_decl); + } if (warned) inform (DECL_SOURCE_LOCATION (old_decl), diff --git a/gcc/common.opt b/gcc/common.opt index fa1c036..17d54e7 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -661,7 +661,15 @@ Warn about returning a pointer/reference to a local or temporary variable. Wshadow Common Var(warn_shadow) Warning -Warn when one local variable shadows another. +Warn when one variable shadows another. + +Wshadow-local +Common Var(warn_shadow_local) Warning EnabledBy(Wshadow) +Warn when one local variable shadows another local variable or parameter. + +Wshadow-compatible-local +Common Var(warn_shadow_compatible_local) Warning EnabledBy(Wshadow-local) +Warn when one local variable shadows another local variable or parameter of compatible type. Wstack-protector Common Var(warn_stack_protect) Warning diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 022ab6a..291a7ea 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -1195,19 +1195,39 @@ pushdecl_maybe_friend_1 (tree x, bool is_friend) nowarn = true; } - if (warn_shadow && !nowarn) + if ((warn_shadow + || warn_shadow_local + || warn_shadow_compatible_local) + && !nowarn) { bool warned; + enum opt_code warning_code; + /* 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. */ + if (can_convert (TREE_TYPE (oldlocal), TREE_TYPE (x), + tf_none)) + warning_code = OPT_Wshadow_compatible_local; + else + warning_code = OPT_Wshadow_local; if (TREE_CODE (oldlocal) == PARM_DECL) - warned = warning_at (input_location, OPT_Wshadow, + warned = warning_at (input_location, warning_code, "declaration of %q#D shadows a parameter", x); else if (is_capture_proxy (oldlocal)) - warned = warning_at (input_location, OPT_Wshadow, + warned = warning_at (input_location, warning_code, "declaration of %qD shadows a lambda capture", x); else - warned = warning_at (input_location, OPT_Wshadow, + warned = warning_at (input_location, warning_code, "declaration of %qD shadows a previous local", x); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index b2eaea7..e23ca52 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -290,6 +290,7 @@ Objective-C and Objective-C++ Dialects}. -Wpointer-arith -Wno-pointer-to-int-cast @gol -Wno-pragmas -Wredundant-decls -Wno-return-local-addr @gol -Wreturn-type -Wsequence-point -Wshadow -Wno-shadow-ivar @gol +-Wshadow-compatible-local -Wshadow-local @gol -Wshift-overflow -Wshift-overflow=@var{n} @gol -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol -Wsign-compare -Wsign-conversion -Wfloat-conversion @gol @@ -5001,6 +5002,46 @@ explicit typedef, but not if it shadows a struct/class/enum. Do not warn whenever a local variable shadows an instance variable in an Objective-C method. +@item -Wshadow-local +@opindex Wshadow-local +@opindex Wno-shadow-local +Warn when a local variable shadows another local variable or parameter. +This warning is enabled by @option{-Wshadow}. + +@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: + +@smallexample +@group +for (SomeIterator i = SomeObj.begin(); i != SomeObj.end(); ++i) +@{ + for (int i = 0; i < N; ++i) + @{ + ... + @} + ... +@} +@end group +@end smallexample + +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. + +This warning is enabled by @option{-Wshadow-local}. + @item -Wlarger-than=@var{len} @opindex Wlarger-than=@var{len} @opindex Wlarger-than-@var{len} diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C b/gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C new file mode 100644 index 0000000..b0422fa --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wshadow-compatible-local-1.C @@ -0,0 +1,63 @@ +/* { dg-do compile } */ +/* { dg-options -Wshadow-compatible-local } */ + +class Bar { +}; + +class ChildBar : public Bar { +}; + +Bar bar; + +class Foo { + private: + int val; + + public: + int func1(int x) { + int val; + val = x; + return val; + } + + int func2(int i) { // { dg-message "note: shadowed declaration is here" } + int a = 3; // { dg-message "note: shadowed declaration is here" } + + for (int i = 0; i < 5; ++i) { // { dg-warning "shadows a parameter" } + for (int i = 0; i < 3; ++i) { // { dg-warning "shadows a previous local" } + int a = i; // { dg-warning "shadows a previous local" } + func1(a); + } + } + + return a; + } + + int func3() { + int bar; + float func1 = 0.3; + int f = 5; // { dg-message "note: shadowed declaration is here" } + + if (func1 > 1) { + float f = 2.0; // { dg-warning "shadows a previous local" } + bar = f; + } + else + bar = 1; + return bar; + } + + void func4() { + Bar *bar; // { dg-bogus "shadowed declaration" } + ChildBar *cbp; // { dg-bogus "shadowed declaration" } + Bar *bp; // { dg-message "note: shadowed declaration is here" } + if (val) { + int bar; // { dg-bogus "shadows a previous local" } + Bar *cbp; // { dg-bogus "shadows a previous local" } + ChildBar *bp; // { dg-warning "shadows a previous local" } + func1(bar); + } + } +}; + +// { dg-message "note: shadowed declaration" "" { target *-*-* } 26 } diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-1.C b/gcc/testsuite/g++.dg/warn/Wshadow-local-1.C new file mode 100644 index 0000000..9737e23 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-1.C @@ -0,0 +1,35 @@ +/* { dg-do compile } */ +/* { dg-options -Wshadow-local } */ + +struct status +{ + int member; + void foo2 (); + + inline static int foo3 (int member) + { + return member; + } +}; + +int decl1; // { dg-bogus "shadowed declaration" } +int decl2; // { dg-bogus "shadowed declaration" } +void foo (struct status &status, + double decl1) // { dg-bogus "shadows a global" } +{ +} + +void foo1 (int d) +{ + double d; // { dg-error "shadows a parameter" } +} + +void status::foo2 () +{ + int member; // { dg-bogus "shadows a member" } + int decl2; // { dg-bogus "shadows a global" } + int local; // { dg-message "note: shadowed declaration is here" } + { + int local; // { dg-warning "shadows a previous local" } + } +} diff --git a/gcc/testsuite/g++.dg/warn/Wshadow-local-2.C b/gcc/testsuite/g++.dg/warn/Wshadow-local-2.C new file mode 100644 index 0000000..c0d6741 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wshadow-local-2.C @@ -0,0 +1,63 @@ +/* { dg-do compile } */ +/* { dg-options -Wshadow-local } */ + +class Bar { +}; + +class ChildBar : public Bar { +}; + +Bar bar; // { dg-bogus "shadowed declaration" } + +class Foo { + private: + int val; + + public: + int func1(int x) { + int val; // { dg-bogus "shadows a member" } + val = x; + return val; + } + + int func2(int i) { // { dg-message "shadowed declaration is here" } + int a = 3; // { dg-message "shadowed declaration is here" } + + for (int i = 0; i < 5; ++i) { // { dg-warning "shadows a parameter" } + for (int i = 0; i < 3; ++i) { // { dg-warning "shadows a previous local" } + int a = i; // { dg-warning "shadows a previous local" } + func1(a); + } + } + + return a; + } + + int func3() { + int bar; // { dg-bogus "shadows a global" } + float func1 = 0.3; // { dg-bogus "shadows a member" } + int f = 5; // { dg-message "shadowed declaration is here" } + + if (func1 > 1) { + float f = 2.0; // { dg-warning "shadows a previous local" } + bar = f; + } + else + bar = 1; + return bar; + } + + void func4() { + Bar *bar; // { dg-message "shadowed declaration is here" } + ChildBar *cbp; // { dg-message "shadowed declaration is here" } + Bar *bp; // { dg-message "shadowed declaration is here" } + if (val) { + int bar; // { dg-warning "shadows a previous local" } + Bar *cbp; // { dg-warning "shadows a previous local" } + ChildBar *bp; // { dg-warning "shadows a previous local" } + func1(bar); + } + } +}; + +// { dg-message "shadowed declaration is here" "" { target *-*-* } 26 } diff --git a/gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c b/gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c new file mode 100644 index 0000000..80f43de --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-compatible-local-1.c @@ -0,0 +1,36 @@ +/* { dg-do compile } */ +/* { dg-options "-Wshadow-compatible-local" } */ + +struct Bar { +}; + +struct Bar bar; /* { dg-bogus "shadowed declaration" } */ + +int val; /* { dg-bogus "shadowed declaration" } */ + +int func1(int x) { /* { dg-bogus "shadowed declaration" } */ + int val; /* { dg-bogus "shadows a global" } */ + val = x; + return val; +} + +int func2(int i) { + int a = 3; /* { dg-message "shadowed declaration" } */ + int j; /* { dg-message "shadowed declaration" } */ + + for (j = 0; j < i; ++j) { + int a = j; /* { dg-warning "shadows a previous local" } */ + int j = a + 1; /* { dg-warning "shadows a previous local" } */ + func1(j); + } + + return a; +} + +void func4() { + struct Bar bar; /* { dg-bogus "shadowed declaration" } */ + if (val) { + int bar; /* { dg-bogus "shadows a previous local" } */ + func1(bar); + } +} diff --git a/gcc/testsuite/gcc.dg/Wshadow-local-1.c b/gcc/testsuite/gcc.dg/Wshadow-local-1.c new file mode 100644 index 0000000..df70c11 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-local-1.c @@ -0,0 +1,22 @@ +/* { dg-do compile } */ +/* { dg-options "-Wshadow-local" } */ + +int decl1; /* should not warn */ +void foo (double decl1) /* should not warn */ +{ +} + +void foo2 (int d) /* { dg-message "shadowed declaration" } */ +{ + { + double d; /* { dg-warning "shadows a parameter" } */ + } +} + +void foo3 () +{ + int local; /* { dg-message "shadowed declaration" } */ + { + int local; /* { dg-warning "shadows a previous local" } */ + } +} diff --git a/gcc/testsuite/gcc.dg/Wshadow-local-2.c b/gcc/testsuite/gcc.dg/Wshadow-local-2.c new file mode 100644 index 0000000..90d4e0f --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-local-2.c @@ -0,0 +1,49 @@ +/* { dg-do compile } */ +/* { dg-options "-Wshadow-local" } */ + +struct Bar { +}; + +struct Bar bar; /* { dg-bogus "shadowed declaration" } */ + +int val; /* { dg-bogus "shadowed declaration" } */ + +int func1(int x) { /* { dg-bogus "shadowed declaration" } */ + int val; /* { dg-bogus "shadows a global" } */ + val = x; + return val; +} + +int func2(int i) { + int a = 3; /* { dg-message "shadowed declaration" } */ + int j; /* { dg-message "shadowed declaration" } */ + + for (j = 0; j < i; ++j) { + int a = j; /* { dg-warning "shadows a previous local" } */ + int j = a + 1; /* { dg-warning "shadows a previous local" } */ + func1(j); + } + + return a; +} + +int func3() { + int bar; /* { dg-bogus "shadows a global" } */ + float func1 = 0.3; /* { dg-bogus "shadows a global" } */ + + if (func1 > 1) + bar = 2; + else + bar = 1; + return bar; +} + +void func4() { + struct Bar bar; /* { dg-message "shadowed declaration" } */ + if (val) { + int bar; /* { dg-warning "shadows a previous local" } */ + func1(bar); + } +} + +/* { dg-bogus "shadows a global" "" { target *-*-* } 42 } */ diff --git a/gcc/testsuite/gcc.dg/Wshadow-local-3.c b/gcc/testsuite/gcc.dg/Wshadow-local-3.c new file mode 100644 index 0000000..429df37 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wshadow-local-3.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-Wno-shadow" } */ + +void func() { + int i; + { + int i; /* should not warn */ + } +} -- 1.8.3.1
