[C++] PR 68374 ("G++ -Wshadow doesn't warn about static member shadowing")

Message ID 7e3c9162-07af-320d-c910-efaf65d86883@oracle.com
State New
Headers show
Series
  • [C++] PR 68374 ("G++ -Wshadow doesn't warn about static member shadowing")
Related show

Commit Message

Paolo Carlini April 23, 2018, 9:17 p.m.
Hi,

this issue is by and large fixed in trunk, thus I wanted to resolve it 
as such, marking it as fixed for 8.1.0. However the text of the warning 
is misleading / wrong:

     declaration of ‘mVar’ shadows a previous local

where in fact the shadowed declaration isn't local, is a static data 
member of the class. It seems to me that we should simply not handle 
such static old decls in the conditional block handling locals shadowing 
other locals, thus handle those below, together with other non-static 
member declarations. Tested x86-64-linux.

Thanks, Paolo.

///////////////////
/cp
2018-04-23  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/68374
	* name-lookup.c (check_local_shadow): Don't handle static old
	declarations in the block handling locals shadowing locals.

/testsuite
2018-04-23  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/68374
	* g++.dg/warn/Wshadow-13.C: New.

Comments

Jason Merrill April 23, 2018, 9:43 p.m. | #1
On Mon, Apr 23, 2018 at 5:17 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,

>

> this issue is by and large fixed in trunk, thus I wanted to resolve it as

> such, marking it as fixed for 8.1.0. However the text of the warning is

> misleading / wrong:

>

>     declaration of ‘mVar’ shadows a previous local

>

> where in fact the shadowed declaration isn't local, is a static data member

> of the class. It seems to me that we should simply not handle such static

> old decls in the conditional block handling locals shadowing other locals,

> thus handle those below, together with other non-static member declarations.

> Tested x86-64-linux.


!TREE_STATIC seems like the wrong test here; that's also true for
static local variables.

Maybe check DECL_FUNCTION_SCOPE_P instead?

Jason
Paolo Carlini April 23, 2018, 11 p.m. | #2
Hi,

On 23/04/2018 23:43, Jason Merrill wrote:
> On Mon, Apr 23, 2018 at 5:17 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:

>> Hi,

>>

>> this issue is by and large fixed in trunk, thus I wanted to resolve it as

>> such, marking it as fixed for 8.1.0. However the text of the warning is

>> misleading / wrong:

>>

>>      declaration of ‘mVar’ shadows a previous local

>>

>> where in fact the shadowed declaration isn't local, is a static data member

>> of the class. It seems to me that we should simply not handle such static

>> old decls in the conditional block handling locals shadowing other locals,

>> thus handle those below, together with other non-static member declarations.

>> Tested x86-64-linux.

> !TREE_STATIC seems like the wrong test here; that's also true for

> static local variables.

>

> Maybe check DECL_FUNCTION_SCOPE_P instead?

Indeed. I don't know how I mis-analyzed that block to conclude that 
previous static locals weren't handled at all there.

Anyway, I added a testcase for that and I'm running again the full 
testsuite (g++.dg is Ok). Ok if it passes?

Thanks!
Paolo.

//////////////////////
Index: cp/name-lookup.c
===================================================================
--- cp/name-lookup.c	(revision 259571)
+++ cp/name-lookup.c	(working copy)
@@ -2638,6 +2638,7 @@ check_local_shadow (tree decl)
 	  || (TREE_CODE (old) == TYPE_DECL
 	      && (!DECL_ARTIFICIAL (old)
 		  || TREE_CODE (decl) == TYPE_DECL)))
+      && DECL_FUNCTION_SCOPE_P (old)
       && (!DECL_ARTIFICIAL (decl)
 	  || DECL_IMPLICIT_TYPEDEF_P (decl)
 	  || (VAR_P (decl) && DECL_ANON_UNION_VAR_P (decl))))
Index: testsuite/g++.dg/warn/Wshadow-13.C
===================================================================
--- testsuite/g++.dg/warn/Wshadow-13.C	(nonexistent)
+++ testsuite/g++.dg/warn/Wshadow-13.C	(working copy)
@@ -0,0 +1,8 @@
+// PR c++/68374
+// { dg-options "-Wshadow" }
+
+class f {
+  static int mVar;  // { dg-message "shadowed declaration" }
+  int g(int x) { int mVar=3; return x+mVar; }  // { dg-warning "shadows a member of 'f'" }
+};
+int f::mVar = 1;
Index: testsuite/g++.dg/warn/Wshadow-14.C
===================================================================
--- testsuite/g++.dg/warn/Wshadow-14.C	(nonexistent)
+++ testsuite/g++.dg/warn/Wshadow-14.C	(working copy)
@@ -0,0 +1,10 @@
+// PR c++/68374
+// { dg-options "-Wshadow" }
+
+void foo ()
+{
+  static int i;  // { dg-message "shadowed declaration" }
+  {
+    int i;  // { dg-warning "shadows a previous local" }
+  }
+}
Paolo Carlini May 2, 2018, 5:33 p.m. | #3
Hi all, Jason,

On 24/04/2018 01:00, Paolo Carlini wrote:
> On 23/04/2018 23:43, Jason Merrill wrote:

>> On Mon, Apr 23, 2018 at 5:17 PM, Paolo Carlini 

>> <paolo.carlini@oracle.com> wrote:

>>> Hi,

>>>

>>> this issue is by and large fixed in trunk, thus I wanted to resolve 

>>> it as

>>> such, marking it as fixed for 8.1.0. However the text of the warning is

>>> misleading / wrong:

>>>

>>>      declaration of ‘mVar’ shadows a previous local

>>>

>>> where in fact the shadowed declaration isn't local, is a static data 

>>> member

>>> of the class. It seems to me that we should simply not handle such 

>>> static

>>> old decls in the conditional block handling locals shadowing other 

>>> locals,

>>> thus handle those below, together with other non-static member 

>>> declarations.

>>> Tested x86-64-linux.

>> !TREE_STATIC seems like the wrong test here; that's also true for

>> static local variables.

>>

>> Maybe check DECL_FUNCTION_SCOPE_P instead?

> Indeed. I don't know how I mis-analyzed that block to conclude that 

> previous static locals weren't handled at all there.

>

> Anyway, I added a testcase for that and I'm running again the full 

> testsuite (g++.dg is Ok). Ok if it passes?

Is the amended patch Ok for trunk, now? To be clear: fully tested on 
x86_64-linux.

     https://gcc.gnu.org/ml/gcc-patches/2018-04/msg01071.html

Thanks!
Paolo.
Jason Merrill May 2, 2018, 6:01 p.m. | #4
OK.

On Wed, May 2, 2018 at 1:33 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi all, Jason,

>

> On 24/04/2018 01:00, Paolo Carlini wrote:

>>

>> On 23/04/2018 23:43, Jason Merrill wrote:

>>>

>>> On Mon, Apr 23, 2018 at 5:17 PM, Paolo Carlini <paolo.carlini@oracle.com>

>>> wrote:

>>>>

>>>> Hi,

>>>>

>>>> this issue is by and large fixed in trunk, thus I wanted to resolve it

>>>> as

>>>> such, marking it as fixed for 8.1.0. However the text of the warning is

>>>> misleading / wrong:

>>>>

>>>>      declaration of ‘mVar’ shadows a previous local

>>>>

>>>> where in fact the shadowed declaration isn't local, is a static data

>>>> member

>>>> of the class. It seems to me that we should simply not handle such

>>>> static

>>>> old decls in the conditional block handling locals shadowing other

>>>> locals,

>>>> thus handle those below, together with other non-static member

>>>> declarations.

>>>> Tested x86-64-linux.

>>>

>>> !TREE_STATIC seems like the wrong test here; that's also true for

>>> static local variables.

>>>

>>> Maybe check DECL_FUNCTION_SCOPE_P instead?

>>

>> Indeed. I don't know how I mis-analyzed that block to conclude that

>> previous static locals weren't handled at all there.

>>

>> Anyway, I added a testcase for that and I'm running again the full

>> testsuite (g++.dg is Ok). Ok if it passes?

>

> Is the amended patch Ok for trunk, now? To be clear: fully tested on

> x86_64-linux.

>

>     https://gcc.gnu.org/ml/gcc-patches/2018-04/msg01071.html

>

> Thanks!

> Paolo.

Patch

Index: cp/name-lookup.c
===================================================================
--- cp/name-lookup.c	(revision 259571)
+++ cp/name-lookup.c	(working copy)
@@ -2638,6 +2638,7 @@  check_local_shadow (tree decl)
 	  || (TREE_CODE (old) == TYPE_DECL
 	      && (!DECL_ARTIFICIAL (old)
 		  || TREE_CODE (decl) == TYPE_DECL)))
+      && !TREE_STATIC (old)
       && (!DECL_ARTIFICIAL (decl)
 	  || DECL_IMPLICIT_TYPEDEF_P (decl)
 	  || (VAR_P (decl) && DECL_ANON_UNION_VAR_P (decl))))
Index: testsuite/g++.dg/warn/Wshadow-13.C
===================================================================
--- testsuite/g++.dg/warn/Wshadow-13.C	(nonexistent)
+++ testsuite/g++.dg/warn/Wshadow-13.C	(working copy)
@@ -0,0 +1,8 @@ 
+// PR c++/68374
+// { dg-options "-Wshadow" }
+
+class f {
+  static int mVar;  // { dg-message "shadowed declaration" }
+  int g(int x) { int mVar=3; return x+mVar; }  // { dg-warning "shadows a member of 'f'" }
+};
+int f::mVar = 1;