This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[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 <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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]