[google] Add two new -Wshadow warnings

[Diego Novillo, 2011-04-29 14:56:14]

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  <lcwu@google.com>

        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  <lcwu@google.com>

	* 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  <lcwu@google.com>

	* 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 */
+    }
+}
-- 
1.7.3.1


--
This patch is available for review at http://codereview.appspot.com/4452058

[Jason Merrill, 2011-05-03 19:59:20]

codereview seems to be having trouble with the invoke.texi patch so that I can't
give inline comments there.  So I'll say it here: the documentation needs to be
clearer about the relationship between the three -Wshadow options, i.e. that
-Wshadow implies -Wshadow-local implies -Wshadow-compatible-local.

http://codereview.appspot.com/4452058/diff/1/gcc/c-decl.c
File gcc/c-decl.c (left):

http://codereview.appspot.com/4452058/diff/1/gcc/c-decl.c#oldcode2584
gcc/c-decl.c:2584: "non-variable", new_decl);
The change in how to express not giving the "previous declaration" warning in
some cases (from 'break' to code duplication) seems gratuitous to me; the new
way doesn't seem clearly better than the old.

http://codereview.appspot.com/4452058/diff/1/gcc/cp/name-lookup.c
File gcc/cp/name-lookup.c (right):

http://codereview.appspot.com/4452058/diff/1/gcc/cp/name-lookup.c#newcode1119
gcc/cp/name-lookup.c:1119: || warn_shadow_compatible_local)
Why change this condition if you aren't changing the warning code below?