Re: Adding -Wshadow=local to gcc build rules

[Jason Merrill <jason@redhat.com>]

On Fri, Sep 20, 2019, 2:21 PM Bernd Edlinger <bernd.edlinger@hotmail.de>
wrote:

> On 9/19/19 12:19 PM, Richard Biener wrote:
> > On Wed, Sep 18, 2019 at 3:09 PM Bernd Edlinger
> > <bernd.edlinger@hotmail.de> wrote:
> >>
> >> Hi,
> >>
> >> I'm currently trying to add -Wshadow=local to the gcc build rules.
> >> I started with -Wshadow, but gave up that idea immediately.
> >>
> >> As you could expect the current code base has plenty of shadowed
> >> local variables.  Most are trivial to resolve, some are less trivial.
> >> I am not finished yet, but it is clear that it will be a rather big
> >> patch.
> >>
> >> I would like to ask you if you agree that would be a desirable step,
> >> in improving code quality in the gcc tree.
> >
> > I wonder if -Wshadow=compatible-local is easier to achieve?
> >
>
> I tried that and it does not make a big difference, while
> it misses for instance:
>
> * gcc/c-family/c-ada-spec.c (dump_ada_macros)
> unsigned char *s, shadowed by const unsigned char *s. :-/
>
> On the other hand, -Wshadow=global is a lot more difficult,
> because we have lots of globals, for instance:
>
> context.h:
> /* The global singleton context aka "g".
>    (the name is chosen to be easy to type in a debugger).  */
> extern gcc::context *g;
>
> But unfortunately Wshadow=local does not find class members
> shadowed by local variable, which happens for instance in
> wide-int
>
> With -Wshadow, I had to change this in wide-int.h:
>
> Index: gcc/wide-int.h
> ===================================================================
> --- gcc/wide-int.h      (revision 275545)
> +++ gcc/wide-int.h      (working copy)
> @@ -648,9 +648,9 @@ namespace wi
>      storage_ref () {}
>      storage_ref (const HOST_WIDE_INT *, unsigned int, unsigned int);
>
> -    const HOST_WIDE_INT *val;
> -    unsigned int len;
> -    unsigned int precision;
> +    const HOST_WIDE_INT *m_val;
> +    unsigned int m_len;
> +    unsigned int m_precision;
>
>
> So this change was not necessary for -Wshadow=local, although
> I would think that, shadowing class members is not much different from
> shadowing a local scope.
>
> Not sure if shadowing class members should be part of -Wshadow=local
> instead of -Wshadow=global.
>

That would make sense to me.

>