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

Reply via email to