diff mbox

fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

Message ID AANLkTimx8hNJcniP8L4GIghipqmbA8JwlsZ0M4ncZUyq@mail.gmail.com
State New
Headers show

Commit Message

Le-Chun Wu June 10, 2010, 10:37 p.m. UTC 
On Fri, Jun 4, 2010 at 8:50 PM, Jason Merrill <jason@redhat.com> wrote:
> On 06/04/2010 09:30 PM, Le-Chun Wu wrote:
>>
>> On Thu, May 27, 2010 at 12:02 PM, Jason Merrill<jason@redhat.com>  wrote:
>
>>> Why only warn if the TREE_CODE happens to match?  I think we might as
>>> well
>>> also warn if, for instance, a typedef shadows a variable.
>
>> The current implementation (before my change) already warns if a
>> typedef shadows a variable. What it doesn't warn is if a local
>> variable shadows a typedef. And this behavior appears to be by design
>> (based on PR/16). The rationale provided in PR/16 is that when a local
>> variable shadows a previously-declared struct/class/enum name, it's
>> probably not accidental but intentional, and therefore the compiler
>> should not warn about it.
>
> Yes, but that's because struct/class/enum names can be hidden by other
> declarations in the same scope, for C compatibility.  We should still warn
> about shadowing an explicit typedef.
>

Sure.. I have revised the patch so that the compiler warns if

1. a local variable shadows another variable, parameter, class member,
or explicit typedef (but not warn if shadows a struct/class/enum name)
2. a type declaration (explicit typedef, struct, class, enum) shadows
another variable, parameter, class member, or type declaration.

This way, we expand the warning to cover type declarations but still
preserve the fix for PR/16.

What do you think?

Thanks,

Le-chun


2010-06-10  Le-Chun Wu  <lcwu@google.com>

       PR/44128
       * gcc/doc/invoke.texi: Update documentation of -Wshadow.
       * gcc/cp/name-lookup.c (pushdecl_maybe_friend): Warn when a local decl
       shadows another local or global decl if both decls have the same
       TREE_CODE.
       * gcc/testsuite/g++.dg/warn/Wshadow-7.C: New test.

Comments

Gerald Pfeifer June 13, 2010, 10:34 a.m. UTC | #1 
On Thu, 10 Jun 2010, Le-Chun Wu wrote:
> -Warn whenever a local variable shadows another local variable, parameter or
> -global variable or whenever a built-in function is shadowed.
> +Warn whenever a local variable or type declaration shadows another variable,
> +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 shadows an explicit typedef.

"warn if it shadows"

When this goes in, it will be nice could you add an update to 
gcc-4.6/changes.html, too, since this will be user visible.

Gerald
Le-Chun Wu June 15, 2010, 8:45 p.m. UTC | #2 
On Sun, Jun 13, 2010 at 3:34 AM, Gerald Pfeifer <gerald@pfeifer.com> wrote:
> On Thu, 10 Jun 2010, Le-Chun Wu wrote:
>> -Warn whenever a local variable shadows another local variable, parameter or
>> -global variable or whenever a built-in function is shadowed.
>> +Warn whenever a local variable or type declaration shadows another variable,
>> +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 shadows an explicit typedef.
>
> "warn if it shadows"

Will fix.

>
> When this goes in, it will be nice could you add an update to
> gcc-4.6/changes.html, too, since this will be user visible.
>

Will do.

I will prepare a revised patch with the above fixes. Thanks,

Le-chun
diff mbox

Patch

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 160582)
+++ gcc/doc/invoke.texi	(working copy)
@@ -3842,8 +3842,10 @@  Do not warn whenever an @samp{#else} or
 @item -Wshadow
 @opindex Wshadow
 @opindex Wno-shadow
-Warn whenever a local variable shadows another local variable, parameter or
-global variable or whenever a built-in function is shadowed.
+Warn whenever a local variable or type declaration shadows another variable,
+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 shadows an explicit typedef.

 @item -Wlarger-than=@var{len}
 @opindex Wlarger-than=@var{len}
Index: gcc/testsuite/g++.dg/warn/Wshadow-7.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wshadow-7.C	(revision 0)
+++ gcc/testsuite/g++.dg/warn/Wshadow-7.C	(revision 0)
@@ -0,0 +1,37 @@ 
+// PR c++/44128
+// { dg-options "-Wshadow" }
+
+typedef long My_ssize_t;  // { dg-warning "shadowed declaration" }
+typedef int Foo;          // { dg-warning "shadowed declaration" }
+struct Bar1 {             // { dg-bogus "shadowed declaration" }
+  int a;
+};
+struct Bar2 {             // { dg-warning "shadowed declaration" }
+  int a;
+};
+
+void func() {
+  typedef int My_ssize_t; // { dg-warning "shadows a global" }
+  typedef char My_Num;    // { dg-warning "shadowed declaration" }
+  {
+    typedef short My_Num; // { dg-warning "shadows a previous local" }
+  }
+  int Foo;                // { dg-warning "shadows a global" }
+  float Bar1;             // { dg-bogus "shadows a global" }
+  struct Bar2 {           // { dg-warning "shadows a global" }
+    int a;
+  };
+  struct Bar3 {           // { dg-warning "shadowed declaration" }
+    int a;
+  };
+  struct Bar4 {           // { dg-bogus "shadowed declaration" }
+    int a;
+  };
+  {
+    struct Bar3 {         // { dg-warning "shadows a previous local" }
+      int a;
+    };
+    char Bar4;            // { dg-bogus "shadows a previous local" }
+    int My_Num;           // { dg-warning "shadows a previous local" }
+  }
+}
Index: gcc/cp/name-lookup.c
===================================================================
--- gcc/cp/name-lookup.c	(revision 160582)
+++ gcc/cp/name-lookup.c	(working copy)
@@ -1017,10 +1017,22 @@  pushdecl_maybe_friend (tree x, bool is_f
 		   /* Inline decls shadow nothing.  */
 		   && !DECL_FROM_INLINE (x)
 		   && (TREE_CODE (oldlocal) == PARM_DECL
-		       || TREE_CODE (oldlocal) == VAR_DECL)
-		   /* Don't check the `this' parameter.  */
-		   && !DECL_ARTIFICIAL (oldlocal)
-		   && !DECL_ARTIFICIAL (x))
+		       || TREE_CODE (oldlocal) == VAR_DECL
+                       /* If the old decl is a type decl, only warn if the
+                          old decl is an explicit typedef or if both the old
+                          and new decls are type decls.  */
+                       || (TREE_CODE (oldlocal) == TYPE_DECL
+                           && (!DECL_ARTIFICIAL (oldlocal)
+                               || TREE_CODE (x) == TYPE_DECL)))
+		   /* Don't check the `this' parameter or internally generated
+                      vars unless it's an implicit typedef (see
+                      create_implicit_typedef in decl.c).  */
+		   && (!DECL_ARTIFICIAL (oldlocal)
+                       || DECL_IMPLICIT_TYPEDEF_P (oldlocal))
+                   /* Don't check for internally generated vars unless
+                      it's an implicit typedef (see create_implicit_typedef
+                      in decl.c).  */
+		   && (!DECL_ARTIFICIAL (x) || DECL_IMPLICIT_TYPEDEF_P (x)))
 	    {
 	      bool nowarn = false;

@@ -1081,10 +1093,12 @@  pushdecl_maybe_friend (tree x, bool is_f

 	  /* Maybe warn if shadowing something else.  */
 	  else if (warn_shadow && !DECL_EXTERNAL (x)
-	      /* No shadow warnings for internally generated vars.  */
-	      && ! DECL_ARTIFICIAL (x)
-	      /* No shadow warnings for vars made for inlining.  */
-	      && ! DECL_FROM_INLINE (x))
+                   /* No shadow warnings for internally generated vars unless
+                      it's an implicit typedef (see create_implicit_typedef
+                      in decl.c).  */
+                   && (! DECL_ARTIFICIAL (x) || DECL_IMPLICIT_TYPEDEF_P (x))
+                   /* No shadow warnings for vars made for inlining.  */
+                   && ! DECL_FROM_INLINE (x))
 	    {
 	      tree member;

@@ -1103,7 +1117,13 @@  pushdecl_maybe_friend (tree x, bool is_f
 			   x);
 		}
 	      else if (oldglobal != NULL_TREE
-		       && TREE_CODE (oldglobal) == VAR_DECL)
+		       && (TREE_CODE (oldglobal) == VAR_DECL
+                           /* If the old decl is a type decl, only warn if the
+                              old decl is an explicit typedef or if both the
+                              old and new decls are type decls.  */
+                           || (TREE_CODE (oldglobal) == TYPE_DECL
+                               && (!DECL_ARTIFICIAL (oldglobal)
+                                   || TREE_CODE (x) == TYPE_DECL))))
 		/* XXX shadow warnings in outer-more namespaces */
 		{
 		  warning_at (input_location, OPT_Wshadow,