Re: [PATCH 18/348] Fix -Wsahdow warnings

[Andrey Smirnov <andrew dot smirnov at gmail dot com>]

On Fri, Nov 25, 2011 at 5:00 AM, Joel Brobecker <brobecker@adacore.com> wrote:
> Andrey,
>
> I understand your fustration towards the tone of some of the messages.
> Hopefully things will be better from now on.
>
> And I agree that not enabling -Wshadow by default will let the number
> of such conflict increase again over time. But at the same time, now
> that I am seeing the changes that are required to fix these, I am not
> very happy either. ?I mean, I understand that "index" might be part of
> a system's include.

> But "block_found" (or was it "found_block") seems
> quite surprising.

Thanks for bringing that up, that is, IMHO, an excellent example.
The `block_found' in function conflicts with global variable declared
in symtab.c and it is not that just their names match, types are very
similar too one is `struct block **' another is `const struct *block'.

But for the sake of argument let's assume that this will never cause
any bugs. I say this shadowing still should be resolved because it
makes source code, at least in my experience, harder to understand: on
one hand in `ada_lookup_encoded_symbol' `block_found' is function
parameter used to return result, on the other hand in
`standard_lookup' it is a global variable from symtab.c, rather confusing.

I say the whole situation is exactly why -Wshadow should be enabled.

> Add the fact that includes and compiler vary from
> system to system, and we're not sure that once clean on one machine,
> it'll be clean everywhere else. All of this to fix warnings that,
> as far as I could tell for the most part, did not indicate an actual
> bug in the code.

I've been humbled by the compiler and the machine too many times to
think my bug spotting capabilities for all intents and purposes are
any greater than zero, so my perspective at this is buggy until
formally proven to be bug-free. And from my view your argument sounds
really different: It means that we potentially can have a bug that is
only reproducible on a certain mix of a compiler and the platform, now
I don't mind fixing very well reproducible bugs, it this kind of
once-in-a-blue-moon kind of bugs that I hate very passionately.

> This is why I am left wondering (meaning I haven't decided yet)
> whether the idea of enabling -Wshadow was such a good idea after
> all. I know that looking at the warnings allowed you to spot some
> areas where there definitely is a mistake, and so that's useful.
> I'm not disputing that.

But that tool is only available if the source tree in question can be
compiled with -Wshadow, otherwise it is "checking for possible mistakes
up until, in the order of source file compilation, the first
legitimate shadowing".

> But I'm not convinced by a good number of the patches I've seen, and
> I still haven't decided whether to accept the situation and approve
> them, or not. For that, I asked everyone else' opinion.

Well, now that I made my case, I hope you reserve negative judgment
until the whole patchset is presented.

Andrey Smirnov.