From patchwork Fri Apr 29 14:55:46 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Diego Novillo X-Patchwork-Id: 93433 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id BE65BB6F71 for ; Sat, 30 Apr 2011 00:56:35 +1000 (EST) Received: (qmail 27063 invoked by alias); 29 Apr 2011 14:56:34 -0000 Received: (qmail 27053 invoked by uid 22791); 29 Apr 2011 14:56:32 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, SPF_HELO_PASS, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.44.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 29 Apr 2011 14:56:16 +0000 Received: from hpaq14.eem.corp.google.com (hpaq14.eem.corp.google.com [172.25.149.14]) by smtp-out.google.com with ESMTP id p3TEu26L012607; Fri, 29 Apr 2011 07:56:02 -0700 Received: from tobiano.tor.corp.google.com (tobiano.tor.corp.google.com [172.29.41.6]) by hpaq14.eem.corp.google.com with ESMTP id p3TEtnVR004333; Fri, 29 Apr 2011 07:55:49 -0700 Received: by tobiano.tor.corp.google.com (Postfix, from userid 54752) id 0F42BAE1DD; Fri, 29 Apr 2011 10:55:46 -0400 (EDT) To: reply@codereview.appspotmail.com, lcwu@google.com, jason@redhat.com, joseph@codesourcery.com, gcc-patches@gcc.gnu.org Subject: [google] Add two new -Wshadow warnings (issue4452058) Message-Id: <20110429145546.0F42BAE1DD@tobiano.tor.corp.google.com> Date: Fri, 29 Apr 2011 10:55:46 -0400 (EDT) From: dnovillo@google.com (Diego Novillo) X-System-Of-Record: true X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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. OK for trunk? Applied to google/main 2011-04-27 Le-Chun Wu Google ref 39127. * c-decl.c (warn_if_shadowing): Use the warning code corresponding to the given -Wshadow- variant. * common.opt (Wshadow-local): New option. (Wshadow-compatible-local): New option. * invoke.texi: Document Wshadow-local and Wshadow-compatible-local. * opts.c (common_handle_option): Handle OPT_Wshadow and OPT_Wshadow_local. Do not enable Wshadow-local nor Wshadow-compatible-local if Wshadow is disabled. cp/ChangeLog.google-main 2011-04-27 Le-Chun Wu * name-lookup.c (pushdecl_maybe_friend): When emitting a shadowing warning, use the code corresponding to the given -Wshadow- variant. testsuite/ChangeLog.google-main 2011-04-27 Le-Chun Wu * g++.dg/warn/Wshadow-compatible-local-1.C: New. * g++.dg/warn/Wshadow-local-1.C: New. * g++.dg/warn/Wshadow-local-2.C: New. * gcc.dg/Wshadow-compatible-local-1.c: New. * gcc.dg/Wshadow-local-1.c: New. * gcc.dg/Wshadow-local-2.c: New. * gcc.dg/Wshadow-local-3.c: New. diff --git a/gcc/c-decl.c b/gcc/c-decl.c index 112e814..4a6a17d 100644 --- a/gcc/c-decl.c +++ b/gcc/c-decl.c @@ -2565,7 +2565,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. */ @@ -2579,30 +2581,55 @@ warn_if_shadowing (tree new_decl) tree old_decl = b->decl; if (old_decl == error_mark_node) - { - warning (OPT_Wshadow, "declaration of %q+D shadows previous " - "non-variable", new_decl); - break; - } + warning (OPT_Wshadow, "declaration of %q+D shadows previous " + "non-variable", new_decl); else if (TREE_CODE (old_decl) == PARM_DECL) - 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; + warning (warning_code, + "declaration of %q+D shadows a parameter", new_decl); + warning_at (DECL_SOURCE_LOCATION (old_decl), warning_code, + "shadowed declaration is here"); + } else if (DECL_FILE_SCOPE_P (old_decl)) - warning (OPT_Wshadow, "declaration of %q+D shadows a global " - "declaration", new_decl); + { + warning (OPT_Wshadow, "declaration of %q+D shadows a global " + "declaration", new_decl); + warning_at (DECL_SOURCE_LOCATION (old_decl), OPT_Wshadow, + "shadowed declaration is here"); + } else if (TREE_CODE (old_decl) == FUNCTION_DECL && DECL_BUILT_IN (old_decl)) - { - warning (OPT_Wshadow, "declaration of %q+D shadows " - "a built-in function", new_decl); - break; - } + warning (OPT_Wshadow, "declaration of %q+D shadows " + "a built-in function", new_decl); else - warning (OPT_Wshadow, "declaration of %q+D shadows a previous local", - new_decl); - - warning_at (DECL_SOURCE_LOCATION (old_decl), OPT_Wshadow, - "shadowed declaration is here"); + { + 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; + warning (warning_code, + "declaration of %q+D shadows a previous local", + new_decl); + + warning_at (DECL_SOURCE_LOCATION (old_decl), warning_code, + "shadowed declaration is here"); + } break; } diff --git a/gcc/common.opt b/gcc/common.opt index 8ef5693..c0c7bd8 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -573,6 +573,15 @@ Wshadow Common Var(warn_shadow) Warning Warn when one local variable shadows another +Wshadow-local +Common Var(warn_shadow_local) Warning +Warn when one local variable shadows another local variable or parameter + +Wshadow-compatible-local +Common Var(warn_shadow_compatible_local) Warning +Warn when one local variable shadows another local variable or parameter +of compatible type + Wstack-protector Common Var(warn_stack_protect) Warning Warn when not issuing stack smashing protection for some reason diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c index 5d45e40..115437f 100644 --- a/gcc/cp/name-lookup.c +++ b/gcc/cp/name-lookup.c @@ -1080,22 +1080,44 @@ pushdecl_maybe_friend (tree x, bool is_friend) } } - if (warn_shadow && !nowarn) + if ((warn_shadow + || warn_shadow_local + || warn_shadow_compatible_local) + && !nowarn) { + 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))) + warning_code = OPT_Wshadow_compatible_local; + else + warning_code = OPT_Wshadow_local; if (TREE_CODE (oldlocal) == PARM_DECL) - warning_at (input_location, OPT_Wshadow, + warning_at (input_location, warning_code, "declaration of %q#D shadows a parameter", x); else - warning_at (input_location, OPT_Wshadow, + warning_at (input_location, warning_code, "declaration of %qD shadows a previous local", x); - warning_at (DECL_SOURCE_LOCATION (oldlocal), OPT_Wshadow, + warning_at (DECL_SOURCE_LOCATION (oldlocal), warning_code, "shadowed declaration is here"); } } /* Maybe warn if shadowing something else. */ - else if (warn_shadow && !DECL_EXTERNAL (x) + else if ((warn_shadow + || warn_shadow_local + || warn_shadow_compatible_local) + && !DECL_EXTERNAL (x) /* No shadow warnings for internally generated vars unless it's an implicit typedef (see create_implicit_typedef in decl.c). */ diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index cffc70d..fa247b6 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -259,6 +259,7 @@ Objective-C and Objective-C++ Dialects}. -Wpointer-arith -Wno-pointer-to-int-cast @gol -Wredundant-decls -Wreturn-type -Wripa-opt-mismatch @gol -Wself-assign -Wself-assign-non-pod -Wsequence-point -Wshadow @gol +-Wshadow-compatible-local -Wshadow-local @gol -Wsign-compare -Wsign-conversion -Wstack-protector @gol -Wstrict-aliasing -Wstrict-aliasing=n @gol -Wstrict-overflow -Wstrict-overflow=@var{n} @gol @@ -4052,6 +4053,43 @@ parameter, type, or class member (in C++), or whenever a built-in function is shadowed. Note that in C++, the compiler will not warn if a local variable shadows a struct/class/enum, but will warn if it shadows an explicit typedef. +@item -Wshadow-local +@opindex Wshadow-local +@opindex Wno-shadow-local +Warn when a local variable shadows another local variable or parameter. + +@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. + @item -Wlarger-than=@var{len} @opindex Wlarger-than=@var{len} @opindex Wlarger-than-@var{len} diff --git a/gcc/opts.c b/gcc/opts.c index a78e865..f389745 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -1413,6 +1413,15 @@ common_handle_option (struct gcc_options *opts, opts->x_warn_frame_larger_than = value != -1; break; + case OPT_Wshadow: + warn_shadow_local = value; + warn_shadow_compatible_local = value; + break; + + case OPT_Wshadow_local: + warn_shadow_compatible_local = value; + break; + case OPT_Wstrict_aliasing: set_Wstrict_aliasing (opts, value); break; 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..e251b72 --- /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-warning "shadowed declaration" } + int a = 3; // { dg-warning "shadowed declaration" } + + 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-warning "shadowed declaration" } + + 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-warning "shadowed declaration" } + 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-warning "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..24a5bc2 --- /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-warning "shadowed declaration" } + { + 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..ac3951e --- /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-warning "shadowed declaration" } + int a = 3; // { dg-warning "shadowed declaration" } + + 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-warning "shadowed declaration" } + + 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-warning "shadowed declaration" } + ChildBar *cbp; // { dg-warning "shadowed declaration" } + Bar *bp; // { dg-warning "shadowed declaration" } + 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-warning "shadowed declaration" "" { 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..cb21be9 --- /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-warning "shadowed declaration" } */ + int j; /* { dg-warning "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..d0e0dea --- /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-warning "shadowed declaration" } */ +{ + { + double d; /* { dg-warning "shadows a parameter" } */ + } +} + +void foo3 () +{ + int local; /* { dg-warning "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..9d52fac --- /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-warning "shadowed declaration" } */ + int j; /* { dg-warning "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-warning "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 */ + } +}