On 10/7/19 8:56 AM, Richard Biener wrote:
> On Sun, Oct 6, 2019 at 1:24 PM Bernd Edlinger <bernd.edlin...@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.
>>
