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

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> Date: Fri, 25 Nov 2011 17:41:02 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Fri, 25 Nov 2011 16:11:11 +0100 (CET)
> > From: Mark Kettenis <mark.kettenis@xs4all.nl>
> > 
> > > 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.
> 
> But the patches suggested by Andrey didn't find any instances of the
> above.  So I submit that this danger is purely theoretical at this
> point, or at least sufficiently rare in GDB to render this
> consideration not important in practice for us.

You're missing my point.

A significant number of Andrey's changes (all-but-one of the ones I
looked at) rename *local* variables because they have the same name as
a function.  My example above tries to show you that there is
absolutely no problem with that, because if one wouldf accidentally
try to invoke the function that has the same name as the local
variable, the compiler would already generate an error.

> > > 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.
> 
> Clashing with library symbols _is_ a good reason to prevent GDB from
> building.

For *local* variables?  

> > And remove some perfectly usable and meaningful variable names.
> 
> They can be easily replaced by no less meaningful and usable ones.

But such a change requires one to actually read and understand the
code.  Such changes can therefore never be obvious, so they can't be
fixed quickly.

> > > 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!
> 
> The same is true of any code you add or change, with the possible
> exception of very trivial changes.

For most changes, compiling on one system gives you reasonable
confidence that things work on other systems as well.

> > Most people that contribute to GDB are fairly competent programmers.
> 
> Being competent doesn't mean being clairvoyant.  How can we trust
> ourselves to know by heart every single symbol in the standard
> libraries?

Exactly!  That's why -Wshadow is so bad.  Our "defs.h" pulls in lots
of system headers.  And on top of that we use options like
_GNU_SOURCE, which invite the system to pollute the global namespace
as much as it want.

Since there is absolutely no problem with a local variable that has
the same name as a library function, this is bad.