| Message ID | 1473466003-19517-1-git-send-email-mjw@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard <mjw@redhat.com> 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
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 <mjw@redhat.com> 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
On 9/10/16, Ian Lance Taylor <iant@google.com> wrote: > On Fri, Sep 9, 2016 at 5:06 PM, Mark Wielaard <mjw@redhat.com> 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
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; } }
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.