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

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

On 10/7/19 8:56 AM, Richard Biener wrote:
> 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).
> 

I think the easiest way would be to add something like this to all files that
are too difficult to fix right now:

Index: combine.c
===================================================================
--- combine.c	(revision 276634)
+++ combine.c	(working copy)
@@ -107,6 +107,10 @@
 #include "print-rtl.h"
 #include "function-abi.h"
 
+#if __GNUC__ >= 7
+#pragma GCC diagnostic ignored "-Wshadow=local"
+#endif
+
 /* Number of attempts to combine instructions in this function.  */
 
 static int combine_attempts;

I have modified
192 of 453 *.c files in gcc/*.c

and
59 out of 241 files in the frontends, c*/*.c  fortran/*.c lto/*.c etc.

Many of them have only 1-2 changes, so might be possible to fix at
least some of them, but OTOH more than 50% don't have any issues.

> This probably means fixing the header file issues first though.
> 

Yes.


Bernd.

> Richard.
> 
>>
>> Bernd.
>>