Re: [PATCH, OBVIOUS] Fix -Wshadow=local warnings in gcc/[a-c]*.c

[Richard Biener <richard dot guenther at gmail dot com>]

On Sun, Oct 6, 2019 at 1:24 PM Bernd Edlinger <bernd.edlinger@hotmail.de> wrote:
>
> On 10/5/19 8:24 PM, Segher Boessenkool wrote:
> >
> > I am maintainer of combine, I know all about its many problems (it has much
> > deeper problems than this unfortunately).  Thanks for your help though, this
> > is much appreciated, but I do think your current patch is not a step in the
> > right direction.
> >
>
> Hmm, thanks for your open words these are of course important.  I will not
> commit this under obvious rules since you objected to the patch in general.
>
> What I want to achieve is to make sure that new code is not introducing more
> variable shadowing.  New shadowing variables are introduced by new code, unless
> we have a warning enabled.  And the warning need to be enabled together with
> -Werror otherwise it will be overlooked.
>
> For instance I believe MISRA has even stronger coding rules with respect
> to shadowing.
>
> What I tried to do was adding -Wshadow=local to the -Werror warnings set
> and do a more or less mechanical change over the whole code base.
> How that mechanical change is done - if at all -, needs to be agreed upon.
>
> Currently I have the impression that we do not agree if this warning is to be
> enabled at all.

I think if the current code-base was clean then enabling it would be a
no-brainer.
But I agree that mechanically "fixing" the current code-base, while ending up
with no new introductions of local shadowing, is worse if it makes the current
code-base worse.  Consider your

  for  (int i = ....)
    {
...
       {
          int i = ...;
          ... (*)
       }
    }

when editing (*) using 'i' will usually be picking up the correct one.  If you
change the inner do 'i1' then fat-fingering 'i' is easy and bound to happen
and will be silently accepted.  It's also not any more obvious _which_
of the i's is intended since 'i1' is not any more descriptive than 'i'.

If only it will confuse developers familiar with the code then such change is
making things worse.

But yes, this means that the quest to enable -Werror=shadow=local becomes
much harder.  Would it be possible to enable it selectively for "clean"
files in the Makefile rules?  White-listing with -Wno-error=... doesn't work
because older host compilers treat unknown options as error there.  More
configury around this might help (like simply not enabling it during stage1
and using a configure macro in the while-listing makefile rules).

This probably means fixing the header file issues first though.

Richard.

>
> Bernd.
>