This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
- From: Mark Wielaard <mjw at redhat dot com>
- To: gcc-patches at gcc dot gnu dot org
- Cc: Eric Gallager <egall at gwmail dot gwu dot edu>, Ian Lance Taylor <iant at google dot com>, Jason Merrill <jason at redhat dot com>, Paolo Bonzini <bonzini at gnu dot org>, Jim Meyering <jim at meyering dot net>, Diego Novillo <dnovillo at google dot com>, Le-Chun Wu <lcwu at google dot com>, Mark Wielaard <mjw at redhat dot com>
- Date: Sun, 11 Sep 2016 15:02:35 +0200
- Subject: [PATCH] Add -Wshadow-local and -Wshadow-compatible-local.
- Authentication-results: sourceware.org; auth=none
- References: <CAMfHzOszteR0Ky_+7ioFkpu9-K5m8NRozqGJhh0FNGcFDSWPZg@mail.gmail.com>
On Sat, Sep 10, 2016 at 09:51:57AM -0400, Eric Gallager wrote:
> On 9/10/16, Ian Lance Taylor <iant@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 <lcwu@google.com>
Mark Wielaard <mjw@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 <lcwu@google.com>
Mark Wielaard <mjw@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 <lcwu@google.com>
Mark Wielaard <mjw@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