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

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Wed, Nov 30, 2011 at 9:59 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Andrey" == Andrey Smirnov <andrew.smirnov@gmail.com> writes:
>
> Andrey> I hope that kinds of shadowing would still be detectable even with this
> Andrey> patch applied.
>
> Why is that?

Because when somebody decides to modify such a code and rename/remove
local variables, compiler will not warn them about any leftover pieces
of code referencing now renamed/non-existent variable. And on some
occasions that would lead to introduction of weird, not so easy to
reproduce bugs.

Although generally I think functionality introduced by that patch
should have been optional. In my opinion C gives one enough of an
arsenal to shoot their own foot, so while Mark is adamant that scalar
variables will never be the cause of a bug, I remain unconvinced and
still think that even such unlikely cause is still a cause. But then
again it is just my opinion, I do not have real world examples, and it
looks like my synthetic ones are not very persuasive.

> Andrey> It would pretty much solve that problem, yes, but still it would
> Andrey> divide patch submitters into two groups those who have newest gcc and
> Andrey> -Wshadow enabled by default, and those who don't. And the people
> Andrey> without -Wshadow enabled compilers would be, on occasion, breaking the
> Andrey> build because they have no means to check for -Wshadow caused errors.
> Andrey> I hope I missing something and it is not the case, but that how the
> Andrey> things seems to me now.
>
> Yes, I think it would result in some periodic breakage until the newer
> GCC is widely distributed.  I'm willing to put up with that.  We already
> put up with it, in a way, due to other GCC differences... see the
> uninitialized variable patches or FORTIFY_SOURCE patches that go in from
> time to time.
>

But those are pretty much the results we would get with enabling
-Wshadow on all versions of gcc but on a per-platform basis.

BTW I'm just playing Devil's advocate here, IMHO, your solution is
still better than no -Wshadow at all.

> If the configury part is set up properly, then this warning will simply
> auto-enable for people when they upgrade GCC.  So, it isn't like we'll
> just forget about it; more like at some point we'll all be joining in :)

I wish this world would have less small, city-local companies, doing
embedded development whose version of GCC is fixed in stone because
the whole building system is so obscure to anyone that no one dares to
introduce any changes to it, and usually no one even cares. I used to
work in such a place, so I know that some people will be left behind.
:-)

Andrey Smirnov