| 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") | expand |
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
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" } + } +}
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.
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.
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;