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

Mark Kettenis
2011-11-25T15:11:52Z

> Date: Fri, 25 Nov 2011 14:02:55 +0200
> From: Eli Zaretskii <eliz@gnu.org>
>
> > Date: Thu, 24 Nov 2011 14:00:57 -0800
> > From: Joel Brobecker <brobecker@adacore.com>
> > Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, gdb-patches <gdb-patches@sourceware.org>
> >
> > 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.
>
> If it surprised you, it means -Wsahdow did its job well.

No it didn't. It does a poor job, by flagging cases that can never
ever cause problems. A local variable "shadowing" a global function
name will never be a problem, because something like:

void foo(void);

void
bar(void)
{
int foo;

foo();
}

simply won't compile, even if you don't use -Werror.

> IOW, it's easy to avoid well-known names like `printf' or `strchr'.
> It's the not-so-well-known names that give you hell and high water.

Exactly. And since we enable -Werror by default, -Wshadow will break
GDB builds for no good reason. And remove some perfectly usable and
meaningful variable names.

> > 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.
>
> But this is true of any other non-trivial piece of our code. E.g., if
> you declare a variable `long' expecting it to be a 64-bit type, this
> will break on MS-Windows, and you will never no until you actually try
> such a compilation.

Well, that would be a wrong assumption on any sane 32-bit platform as
well, so that issue could be caught on most platforms that GDB runs on.

> In general, we have no bullet-proof way of making sure our code works
> on all supported platforms, except by actually compiling it on those
> platforms. That's why platforms which lose their area maintainers
> bit-rot quite quickly. This compiler switch doesn't change this
> situation in any way.

I disagree. -Wshadow is a game changer because actually compiling on
*all* plaforms is the only way to make sure that I don't actually
break anything. That's extremely bad!

> > 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.

Absolutely!

> It's a bug waiting to happen, though. That it didn't happen yet is
> just sheer luck.

Not really. Most people that contribute to GDB are fairly competent
programmers.