Re: RFC: Use -Wall -Wextra

[Daniel Jacobowitz <drow@false.org>]

On Fri, Dec 29, 2006 at 02:13:16PM +0200, Eli Zaretskii wrote:
> > Date: Thu, 28 Dec 2006 14:55:33 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > I'd like to hear opinions on this patch.  It changes the default set of GDB
> > build warnings from:
> > 
> > build_warnings="-Wimplicit -Wreturn-type -Wcomment -Wtrigraphs \
> > -Wformat -Wparentheses -Wpointer-arith -Wformat-nonliteral \
> > -Wunused-label -Wunused-function -Wno-pointer-sign"
> > 
> > to:
> > 
> > build_warnings="-Wall -Wextra -Wpointer-arith -Wformat-nonliteral \
> > -Wno-pointer-sign -Wno-unused-parameter \
> > -Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers"
> 
> I would agree only if we never try to use -Werror, because with such
> aggressive warnings GDB will never build if we add -Werror.
> 
> My other fear is that, with GCC becoming more and more picky about
> perfectly valid C code, these options will cause the compilation to
> become very noisy, but I guess we will hear complaints if that
> happens.

I don't understand.  I built GDB with these warnings and -Werror, so
that can't be literally true.  I definitely don't want to back away
from -Werror: I think it's done us a lot of good.  Binutils and GCC
already build by default using -Wall -Werror, GCC already uses -Wextra
-Werror, and we already use -Wpointer-arith and -Wformat-nonliteral
with -Werror; only -Wextra is really new for us.

I enabled -Wextra mostly because I wanted -Walways-true; some of the
others are useful too.  For instance, the warning about empty bodies in
if statements is in -Wextra and found a real bug in GDB.  Some of the
-Wextra warnings are a bit dubious, but most of the dubious ones can be
disabled... which is what I've done above.

Can you give me some concrete examples of warnings you're concerned about?

(Remember, we ship releases without -Werror.)

> > I'd really like to turn on -Wunused too, but it has been off for so long
> > that we have a substantial number of unused local variables - it will take
> > some work to clean up.
> 
> I'd advise against -Wunused: the problems it finds are harmless,
> whereas fixing them is not trivial at all, and quite ugly in some
> cases.

I'm afraid I don't understand this either.  The problems -Wunused finds
are usually very easy to fix.  We already use -Wunused-label and
-Wunused-function. (oops!  My patch disabled those because I added
-Wunused at the last minute!  I need to fix that.) That leaves unused
parameters, unused values, and unused local variables. I don't really
agree with the rationale in gdbint about why we don't want the unused
parameter warnings, and binutils made the opposite choice, but in any
case I left it disabled.  And we avoid macros and conditional
compilation, so unused local variables and values are quite easy to
fix.

Anyway, if you want a weaker set of warnings, I'll go for that too.
I can still configure with more aggressive ones here and fix the
problems without them getting in anyone else's way.  -Wunused in
particular - that's basically garbage collection.  I think the average
GDB source file has two unused local variables, and we have a lot
of source files.

-- 
Daniel Jacobowitz
CodeSourcery