Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Jason Merrill]

Sorry, thought I had responded to this already.

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.

Jason

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Le-Chun Wu]

On Thu, May 27, 2010 at 12:02 PM, Jason Merrill <jason@redhat.com> wrote:
> Sorry, thought I had responded to this already.

Sorry for my delay in responding to your response. (Been sidetracked
by other stuff.)

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

What this patch tries to do is for the compiler to warn if a type
declaration shadows another type declaration, or a local struct
shadows another struct, etc. (That's why we check if the new decl and
the old decl have the same TREE_CODE.) These cases are most likely
bugs/typos instead of intentional.

Attached please find the latest patch with the updated source tree. I
also modified the -Wshadow documentation for the new behavior. Please
let me know if you have any other comments. Thanks,

Le-chun

2010-06-04  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.

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi (revision 159995)
+++ gcc/doc/invoke.texi (working copy)
@@ -3834,8 +3834,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 shadows another local variable, parameter,
+global variable, or a class member (in C++) or whenever a built-in function
+is shadowed, or whenever a local declaration (e.g. a type declaration)
+shadows another local or global declaration of the same type.

 @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,12 @@
+// PR c++/44128
+// { dg-options "-Wshadow" }
+
+typedef long My_ssize_t;  // { dg-warning "shadowed declaration" }
+
+void bar() {
+  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" }
+  }
+}
Index: gcc/cp/name-lookup.c
===================================================================
--- gcc/cp/name-lookup.c        (revision 159995)
+++ gcc/cp/name-lookup.c        (working copy)
@@ -1017,7 +1017,8 @@ 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)
+                      || TREE_CODE (oldlocal) == VAR_DECL
+                       || TREE_CODE (oldlocal) == TREE_CODE (x))
                   /* Don't check the `this' parameter.  */
                   && !DECL_ARTIFICIAL (oldlocal)
                   && !DECL_ARTIFICIAL (x))
@@ -1103,7 +1104,8 @@ pushdecl_maybe_friend (tree x, bool is_f
                           x);
                }
              else if (oldglobal != NULL_TREE
-                      && TREE_CODE (oldglobal) == VAR_DECL)
+                      && (TREE_CODE (oldglobal) == VAR_DECL
+                           || TREE_CODE (oldglobal) == TREE_CODE (x)))
                /* XXX shadow warnings in outer-more namespaces */
                {
                  warning_at (input_location, OPT_Wshadow,

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Jason Merrill]

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.

Jason

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Le-Chun Wu]

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.

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,

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Gerald Pfeifer]

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

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Le-Chun Wu]

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

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Le-Chun Wu]

Here is the revised patch that fixes the wording in invoke.texi. I
also enclosed at the bottom of this email a new entry in
gcc-4.6/changes.html for -Wshadow which will be checked in after the
patch is submitted.

Bootstrapped and tested on x86_64-linux-gnu. OK for trunk?

Thanks,

Le-chun


2010-06-23  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 (variable or type) shadows another type.
        * gcc/testsuite/g++.dg/warn/Wshadow-7.C: New test.

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 161236)
+++ gcc/doc/invoke.texi	(working copy)
@@ -3852,8 +3852,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 it 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 161236)
+++ 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,


********************************************************************

Index: htdocs/gcc-4.6/changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.6/changes.html,v
retrieving revision 1.18
diff -u -r1.18 changes.html
--- htdocs/gcc-4.6/changes.html 13 Jun 2010 12:44:28 -0000      1.18
+++ htdocs/gcc-4.6/changes.html 23 Jun 2010 22:48:38 -0000
@@ -83,6 +83,11 @@
     function with such an exception specification.  This dramatically
     reduces or eliminates the code size overhead from adding the exception
     specification.</li>
+
+    <li> The <code>-Wshadow</code> option now warns if a local variable or
+    type declaration shadows another type in C++. Note that the compiler will
+    not warn if a local variable shadows a struct/class/enum, but will warn
+    if it shadows an explicit typedef. </li>
   </ul>

   <h4>Runtime Library (libstdc++)</h4>

Re: [patch] fix PR/44128 (C++ not warn on type decl shadowing with -Wshadow)

[Jason Merrill]

OK.

Jason