Using -Wshadow

Neil Conway <neilc@samurai.com> writes:

GCC supports the -Wshadow command-line option:
-Wshadow
Warn whenever a local variable shadows another local
variable, parameter or global variable or whenever a
built-in function is shadowed.

Currently, enabling this for the PostgreSQL tree produces a lot of
warnings. Would anyone object if I corrected these usages of a
shadowed local variable, and then enabled this warning flag for
standard GCC builds?

How many is "a lot"? What are the odds that this would produce spurious
warnings on some platforms due to shadowing of platform-specific
functions or globals?

I wouldn't object to something that catches shadowings of parameters or
local variables, but I think the flag as defined is not very useful.

If there are any other GCC warning flags anyone else feels would be
useful, let me know.

I have for a long time wanted to enable -Wcast-align and -Wpointer-arith,
but so far the tedium of getting rid of the warnings exceeds my
enthusiasm for it.

Another nice thing would be to get rid of the warnings about casting
between "char" and "unsigned char" that pop up on many (most?) non-gcc
compilers. Most of the occurrences are in or near the multibyte stuff,
so maybe this could be coordinated somehow with Peter's plans for
upgrading locale support.

regards, tom lane

On Mon, Nov 24, 2003 at 12:25:32PM -0500, Neil Conway wrote:

If there are any other GCC warning flags anyone else feels would be
useful, let me know.

I find the following also useful:
-Wcast-align
-Wcast-qual
-Wpointer-arith
-Wstrict-prototypes

And maybe:
-Waggregate-return

Kurt

Tom Lane <tgl@sss.pgh.pa.us> writes:

Neil Conway <neilc@samurai.com> writes:

Currently, enabling this for the PostgreSQL tree produces a lot of
warnings.

How many is "a lot"?

Maybe a couple hundred or so, but most of the warnings are derived
from a few globals with common names -- I would guess it won't be too
much trouble to fix...

What are the odds that this would produce spurious warnings on some
platforms due to shadowing of platform-specific functions or
globals?

No idea -- I'm content to jump that bridge (e.g. by removing -Wshadow
if necessary) when we come to it.

Ok, I'll work on this and submit a patch to -patches when I'm
done. I'll leave the work on the other warning flags you mentioned for
someone else.

-Neil

Tom Lane <tgl@sss.pgh.pa.us> writes:

I wouldn't object to something that catches shadowings of parameters
or local variables, but I think the flag as defined is not very
useful.

On closer examination, that seems like a prescient comment. There are
about 1100 distinct warnings enabled by this flag. Of these, about 900
are caused by shadowed names from system headers: for example, using
'index' or 'shutdown' as the name of a function parameter. Note that a
single instance of shadowing in a header file generates warnings every
time that header file is included, so the number of actual places that
need to be changed should be smaller than 1100.

There are about 200 warnings that are not shadows of something from
the system headers. A fair number of these are also not very useful
(e.g. every variable named "length" triggers a warning, since that
shadows a function defined in pg_list.h), but there are also a fair
number of legitimately-shadowed local variables.

I think this leaves us with three options:

(1) Do nothing

(2) Enable -Wshadow for GCC, fix all the instances of the warning in
the source tree.

(3) Manually scan through the list of warnings and just submit
patches for the legitimate instances of shadowing.

The problem with #2 is the large number of warnings induced by system
headers: other platforms / standard libraries may well cause
additional instances of shadowing, so it might take a little while to
track down all the spurious warnings. On the other hand, it would be
nice if we could just turn this on and then forget about it.

Any comments?

-Neil

Neil Conway writes:

The problem with #2 is the large number of warnings induced by system
headers: other platforms / standard libraries may well cause
additional instances of shadowing, so it might take a little while to
track down all the spurious warnings.

Yes, that's what I'm afraid of. We might be chasing warnings forever.
I'm not sure what the point is anyway. Shadowing is perfectly
well-defined and I've never heard of a real problem because of it.

--
Peter Eisentraut peter_e@gmx.net

Peter Eisentraut <peter_e@gmx.net> writes:

I'm not sure what the point is anyway. Shadowing is perfectly
well-defined and I've never heard of a real problem because of it.

Well, shadowing a formal parameter with a local variable is most likely
a mistake, and shadowing a local with a more-tightly-nested local is,
if not an outright mistake, certain to confuse future maintainers.
So I'd be in favor of getting rid of cases like that.

I can't get excited about forbidding shadowing of globals by locals,
though ... seems like that's practically giving up one of the
advantages of having a block-structured language in the first place.

BTW, what I find by experiment with gcc 2.95.3 is that
local-shadowing-formal is warned of just with -Wall, if the local is
declared at the function's outermost brace level, whether or not you
say -Wshadow. So we already know we have none of those.

regards, tom lane