Re: RFC: Use -Wall -Wextra

[Daniel Jacobowitz <drow@false.org>]

On Fri, Dec 29, 2006 at 10:44:25PM +0200, Eli Zaretskii wrote:
> > I definitely don't want to back away from -Werror: I think it's done
> > us a lot of good.
> 
> I'm not against -Werror, I just don't trust GCC to not flag some
> innocent code as questionable under -Wall -Wextra.  These flags are
> for those who intentionally want a noisy compiler; adding -Werror to
> them is asking for too much trouble.

How about -Wall without -Wextra then?  This is the compromise adopted
by binutils and GCC.

> > I enabled -Wextra mostly because I wanted -Walways-true
> 
> IMO -Walways-true is one of the greatest nuisances with the latest GCC
> versions.  Some code, especially sophisticated macros that need to
> work portably with different data sizes, simply cannot be made to work
> around this warning, especially if you want to handle 32-bit and
> 64-bit machines.  There already are such problems in GNU Make and in
> Emacs, and developers are unwilling to fix them because it's too much
> trouble, if at all possible.

I think neither of us is actually talking about -Walways-true.  It
doesn't seem to control what I thought it did.  Anyway, I was looking
for the "comparison of unsigned >= 0 is always true" warning
specifically.  The only thing controlling that is -Wextra.  I suspect
that's the one you are concerned about, too.

I'd like to enable that warning but I won't cry if we agree not to. It
did find one bug in GDB already, and would have found another if it had
been enabled (the ia64-tdep bug I fixed yesterday).  But I can build
with it locally on systems where I know it isn't a big problem.

> Those issued by -Walways-true and the ones that wine about mismatch
> between pointers to signed and unsigned char are the two that come to
> mind.

I actually think the latter is useful, but not useful enough to
continue fixing up GDB for it - my patch left that one disabled,
deliberately.  Let's not go back there again, it was a real nuisance.

> > > 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.
> 
> You mean with "i = i;"?

No, either by deleting the unused local variable if it's truly unused,
or by adding ATTRIBUTE_UNUSED if it is conditionally unused (or
avoiding conditional compilation).

>     gcc -c -g -O2    -I. -I. -I./config -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I./../include/opcode -I./../readline/.. -I../bfd -I./../bfd -I./../include   -DMI_OUT=1 -DTUI=1  -Wall -Wextra -Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign -Wno-unused-parameter -Wno-unused -Wno-sign-compare -Wno-switch -Wno-missing-field-initializers -Werror infrun.c
>     cc1: warnings being treated as errors
>     infrun.c: In function 'handle_inferior_event':
>     infrun.c:1458: warning: statement with no effect
>     make[2]: *** [infrun.o] Error 1

Thanks.  That comes from the default definition of a macro which no
longer has any non-default definitions; we may as well garbage collect
it.  I don't know why I didn't get the warning; I can provoke it for
a small testcase.

I'll remove the macro, since that's an unrelated cleanup.

-- 
Daniel Jacobowitz
CodeSourcery