-Wshadow warning
Wouldn't -Wshadow be more useful if it obeyed -Wno-system-headers? For code like #include int foo (int atof); int foo (int atof) { return atof; } we currently do not warn on the prototype, but do on the function definition, leading to reports such as http://sourceware.org/bugzilla/show_bug.cgi?id=13121 The following has been bootstrapped and regression tested powerpc-linux. OK to apply? * c-decl.c (warn_if_shadowing): Don't warn if shadowed identifier is from system header. Index: gcc/c-decl.c === --- gcc/c-decl.c(revision 178035) +++ gcc/c-decl.c(working copy) @@ -2516,7 +2516,10 @@ warn_if_shadowing (tree new_decl) /* Is anything being shadowed? Invisible decls do not count. */ for (b = I_SYMBOL_BINDING (DECL_NAME (new_decl)); b; b = b->shadowed) -if (b->decl && b->decl != new_decl && !b->invisible) +if (b->decl && b->decl != new_decl && !b->invisible + && (b->decl == error_mark_node + || diagnostic_report_warnings_p (global_dc, +DECL_SOURCE_LOCATION (b->decl { tree old_decl = b->decl; -- Alan Modra Australia Development Lab, IBM
Re: -Wshadow warning
On Wed, Aug 24, 2011 at 9:32 PM, Alan Modra wrote: > Wouldn't -Wshadow be more useful if it obeyed -Wno-system-headers? > For code like > > #include > int foo (int atof); > int foo (int atof) { return atof; } > > we currently do not warn on the prototype, but do on the function > definition, leading to reports such as > http://sourceware.org/bugzilla/show_bug.cgi?id=13121 > > The following has been bootstrapped and regression tested > powerpc-linux. OK to apply? OK. > > * c-decl.c (warn_if_shadowing): Don't warn if shadowed > identifier is from system header. > > Index: gcc/c-decl.c > === > --- gcc/c-decl.c (revision 178035) > +++ gcc/c-decl.c (working copy) > @@ -2516,7 +2516,10 @@ warn_if_shadowing (tree new_decl) > > /* Is anything being shadowed? Invisible decls do not count. */ > for (b = I_SYMBOL_BINDING (DECL_NAME (new_decl)); b; b = b->shadowed) > - if (b->decl && b->decl != new_decl && !b->invisible) > + if (b->decl && b->decl != new_decl && !b->invisible > + && (b->decl == error_mark_node > + || diagnostic_report_warnings_p (global_dc, > + DECL_SOURCE_LOCATION (b->decl > { > tree old_decl = b->decl; > > > -- > Alan Modra > Australia Development Lab, IBM >
[PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c
valgrind contains a copy of the libiberty demangler which gets compiled with -Wshadow. That shows the following warning: cp-demangle.c: In function ‘d_substitution’: cp-demangle.c:3772:35: warning: declaration of ‘c’ shadows a previous local struct demangle_component *c; ^ cp-demangle.c:3708:8: note: shadowed declaration is here char c; ^ Fix that by renaming the struct demangle_component variable to dc and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this one). libiberty/ChangeLog: * cp-demangle.c (d_substitution): Change struct demangle_component variable name from c to dc. * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow. * configure: Regenerate. diff --git a/libiberty/configure b/libiberty/configure index 0f8e9b9..91a051c 100755 --- a/libiberty/configure +++ b/libiberty/configure @@ -4398,7 +4398,8 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu ac_libiberty_warn_cflags= save_CFLAGS="$CFLAGS" for real_option in -W -Wall -Wwrite-strings -Wc++-compat \ - -Wstrict-prototypes; do + -Wstrict-prototypes \ + -Wshadow; do # Do the check with the no- prefix removed since gcc silently # accepts any -Wno-* option on purpose case $real_option in diff --git a/libiberty/configure.ac b/libiberty/configure.ac index 9d3f298..44bed8f 100644 --- a/libiberty/configure.ac +++ b/libiberty/configure.ac @@ -160,7 +160,8 @@ AC_SYS_LARGEFILE AC_PROG_CPP_WERROR ACX_PROG_CC_WARNING_OPTS([-W -Wall -Wwrite-strings -Wc++-compat \ - -Wstrict-prototypes], [ac_libiberty_warn_cflags]) + -Wstrict-prototypes \ + -Wshadow], [ac_libiberty_warn_cflags]) ACX_PROG_CC_WARNING_ALMOST_PEDANTIC([], [ac_libiberty_warn_cflags]) AC_PROG_CC_C_O diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index 1c2bce2..a843dc3 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -3769,7 +3769,7 @@ d_substitution (struct d_info *di, int prefix) { const char *s; int len; - struct demangle_component *c; + struct demangle_component *dc; if (p->set_last_name != NULL) di->last_name = d_make_sub (di, p->set_last_name, @@ -3785,15 +3785,15 @@ d_substitution (struct d_info *di, int prefix) len = p->simple_len; } di->expansion += len; - c = d_make_sub (di, s, len); + dc = d_make_sub (di, s, len); if (d_peek_char (di) == 'B') { /* If there are ABI tags on the abbreviation, it becomes a substitution candidate. */ - c = d_abi_tags (di, c); - d_add_substitution (di, c); + dc = d_abi_tags (di, dc); + d_add_substitution (di, dc); } - return c; + return dc; } } -- 1.8.3.1
Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c
On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard wrote: > valgrind contains a copy of the libiberty demangler which gets compiled > with -Wshadow. That shows the following warning: > > cp-demangle.c: In function ‘d_substitution’: > cp-demangle.c:3772:35: warning: declaration of ‘c’ shadows a previous local > struct demangle_component *c; >^ > cp-demangle.c:3708:8: note: shadowed declaration is here >char c; > ^ > > Fix that by renaming the struct demangle_component variable to dc > and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this one). > > libiberty/ChangeLog: > > * cp-demangle.c (d_substitution): Change struct demangle_component > variable name from c to dc. > * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow. > * configure: Regenerate. The patch to cp-demangle.c is OK. Thanks. I'm not sure about the patch to configure.ac/configure. The last I looked -Wshadow would warn if a local variable shadows a global variable. That can cause a pointless build break if some system header file defines a global variable that happens to have the same name as a local variable. It's not a likely scenario but I don't see a need to court a build breakage. Ian
Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c
On Sat, Sep 10, 2016 at 01:57:54AM -0700, Ian Lance Taylor wrote: > On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard wrote: > > Fix that by renaming the struct demangle_component variable to dc > > and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this one). > > > > libiberty/ChangeLog: > > > > * cp-demangle.c (d_substitution): Change struct demangle_component > > variable name from c to dc. > > * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow. > > * configure: Regenerate. > > The patch to cp-demangle.c is OK. Thanks. > > I'm not sure about the patch to configure.ac/configure. The last I > looked -Wshadow would warn if a local variable shadows a global > variable. That can cause a pointless build break if some system > header file defines a global variable that happens to have the same > name as a local variable. It's not a likely scenario but I don't see > a need to court a build breakage. OK, pushed with just the cp-demangle.c fix. We'll pick up and shadow warnings when we update the valgrind copy again. Cheers, Mark
Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c
On 9/10/16, Ian Lance Taylor wrote: > On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard wrote: >> valgrind contains a copy of the libiberty demangler which gets compiled >> with -Wshadow. That shows the following warning: >> >> cp-demangle.c: In function ‘d_substitution’: >> cp-demangle.c:3772:35: warning: declaration of ‘c’ shadows a previous >> local >> struct demangle_component *c; >>^ >> cp-demangle.c:3708:8: note: shadowed declaration is here >>char c; >> ^ >> >> Fix that by renaming the struct demangle_component variable to dc >> and add -Wshadow to ac_libiberty_warn_cflags (the only warning is this >> one). >> >> libiberty/ChangeLog: >> >> * cp-demangle.c (d_substitution): Change struct >> demangle_component >> variable name from c to dc. >> * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow. >> * configure: Regenerate. > > The patch to cp-demangle.c is OK. Thanks. > > I'm not sure about the patch to configure.ac/configure. The last I > looked -Wshadow would warn if a local variable shadows a global > variable. That can cause a pointless build break if some system > header file defines a global variable that happens to have the same > name as a local variable. It's not a likely scenario but I don't see > a need to court a build breakage. > > Ian > Maybe if the patch to add -Wshadow-local went in, the configure script could use that instead? For reference: https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01317.html
Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c
On Sun, Nov 06, 2016 at 07:03:34PM +0100, Mark Wielaard wrote: > On Sat, 2016-09-10 at 09:51 -0400, Eric Gallager wrote: > > On 9/10/16, Ian Lance Taylor wrote: > > > I'm not sure about the patch to configure.ac/configure. The last I > > > looked -Wshadow would warn if a local variable shadows a global > > > variable. That can cause a pointless build break if some system > > > header file defines a global variable that happens to have the same > > > name as a local variable. It's not a likely scenario but I don't see > > > a need to court a build breakage. > > > > > Maybe if the patch to add -Wshadow-local went in, the configure script > > could use that instead? > > We now have -Wshadow=local. So the attached patch uses that for > libiberty. Is the attached patch OK to commit? Ping? > From f6f938e8053b6caf393c35c6f96c946073ca9373 Mon Sep 17 00:00:00 2001 > From: Mark Wielaard > Date: Sun, 6 Nov 2016 17:36:06 +0100 > Subject: [PATCH] libiberty: Add -Wshadow=local to warning flags (if > supported). > > libiberty/ChangeLog: > >* configure.ac (ac_libiberty_warn_cflags): Add -Wshadow=local. >* configure: Regenerated. > --- > diff --git a/libiberty/configure b/libiberty/configure > index 0f8e9b9..5c4dda5 100755 > --- a/libiberty/configure > +++ b/libiberty/configure > @@ -4398,7 +4398,8 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu > ac_libiberty_warn_cflags= > save_CFLAGS="$CFLAGS" > for real_option in -W -Wall -Wwrite-strings -Wc++-compat \ > - -Wstrict-prototypes; do > + -Wstrict-prototypes \ > + -Wshadow=local; do ># Do the check with the no- prefix removed since gcc silently ># accepts any -Wno-* option on purpose >case $real_option in > diff --git a/libiberty/configure.ac b/libiberty/configure.ac > index 9d3f298..1aa0c7c 100644 > --- a/libiberty/configure.ac > +++ b/libiberty/configure.ac > @@ -160,7 +160,8 @@ AC_SYS_LARGEFILE > AC_PROG_CPP_WERROR > > ACX_PROG_CC_WARNING_OPTS([-W -Wall -Wwrite-strings -Wc++-compat \ > - -Wstrict-prototypes], [ac_libiberty_warn_cflags]) > + -Wstrict-prototypes \ > + -Wshadow=local], [ac_libiberty_warn_cflags]) > ACX_PROG_CC_WARNING_ALMOST_PEDANTIC([], [ac_libiberty_warn_cflags]) > > AC_PROG_CC_C_O > -- > 1.8.3.1 >
Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c
On Sun, Nov 6, 2016 at 10:03 AM, Mark Wielaard wrote: > > We now have -Wshadow=local. So the attached patch uses that for > libiberty. Is the attached patch OK to commit? This is OK. Thanks. Ian
Re: [PATCH] Fix -Wshadow warning in libiberty/cp-demangle.c
On Sat, 2016-09-10 at 09:51 -0400, Eric Gallager wrote: > On 9/10/16, Ian Lance Taylor wrote: > > I'm not sure about the patch to configure.ac/configure. The last I > > looked -Wshadow would warn if a local variable shadows a global > > variable. That can cause a pointless build break if some system > > header file defines a global variable that happens to have the same > > name as a local variable. It's not a likely scenario but I don't see > > a need to court a build breakage. > > > Maybe if the patch to add -Wshadow-local went in, the configure script > could use that instead? We now have -Wshadow=local. So the attached patch uses that for libiberty. Is the attached patch OK to commit? Thanks, Mark From f6f938e8053b6caf393c35c6f96c946073ca9373 Mon Sep 17 00:00:00 2001 From: Mark Wielaard Date: Sun, 6 Nov 2016 17:36:06 +0100 Subject: [PATCH] libiberty: Add -Wshadow=local to warning flags (if supported). libiberty/ChangeLog: * configure.ac (ac_libiberty_warn_cflags): Add -Wshadow=local. * configure: Regenerated. --- diff --git a/libiberty/configure b/libiberty/configure index 0f8e9b9..5c4dda5 100755 --- a/libiberty/configure +++ b/libiberty/configure @@ -4398,7 +4398,8 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu ac_libiberty_warn_cflags= save_CFLAGS="$CFLAGS" for real_option in -W -Wall -Wwrite-strings -Wc++-compat \ - -Wstrict-prototypes; do + -Wstrict-prototypes \ + -Wshadow=local; do # Do the check with the no- prefix removed since gcc silently # accepts any -Wno-* option on purpose case $real_option in diff --git a/libiberty/configure.ac b/libiberty/configure.ac index 9d3f298..1aa0c7c 100644 --- a/libiberty/configure.ac +++ b/libiberty/configure.ac @@ -160,7 +160,8 @@ AC_SYS_LARGEFILE AC_PROG_CPP_WERROR ACX_PROG_CC_WARNING_OPTS([-W -Wall -Wwrite-strings -Wc++-compat \ - -Wstrict-prototypes], [ac_libiberty_warn_cflags]) + -Wstrict-prototypes \ + -Wshadow=local], [ac_libiberty_warn_cflags]) ACX_PROG_CC_WARNING_ALMOST_PEDANTIC([], [ac_libiberty_warn_cflags]) AC_PROG_CC_C_O -- 1.8.3.1