Adding new gcc warnings

[Russ Allbery]

Julien ÉLIE <julien at trigofacile.com> writes:

> As gcc 4.4.x is currently used for the daily generation of snapshots,
> maybe we could add new warnings for "make warnings" builds.

The system will get upgraded to wheezy as soon as I have some time,
although based on current trends that might still be a few more months.

> I suggest the following flags:
> -Wformat-y2k

I recommend -Wformat=2 instead.  It includes that and a bunch of other
useful stuff.

> -Winit-self

I always left this off because if I were to ever do that, it would be
intentional; it's a hard mistake to make by accident.  But I don't recall
ever using it, so I have no objections.

> -Wold-style-definition

Oh, huh, that's not part of the default set?

> -Wmissing-declarations

I use -Wmissing-prototypes.  I'm not sure what the difference is.  The
documentation is not particularly illuminating.

> -Wnormalized=nfc
> -Wpacked
> -Winline
> -Wsync-nand
> -Wvla

These all seem fine, although probably won't trigger with our code (except
for the -Wpacked point that you note below).

> Should -Wswitch-default be added?  It implies that all switch statements
> are required to have a default case, even though all possible cases are
> correctly handled.

I do tend to use this, but it can be quite annoying if you have large
enums and you're not currently handling every case.  Basically, the more
you intentionally use default, the more annoying this is.

> Should -Wfloat-equal be added?  Errors like these ones appear, and
> I wonder whether a fix is needed.
> In innd/status.c, size is a float:
>   if (!size) size = 1; /* avoid divide by zero here too */

Since this is avoiding division by zero, checking against epsilon probably
isn't really needed, but it also probably wouldn't be hard.  IIRC, this is
only a float due to possible calculation size problems with a long.

> In innfeed/host.c:
>   if (h->params->dynBacklogFilter != oldBacklogFilter)

This should really be checking against an epsilon.

> -Wunreachable-code, -Wstrict-overflow=2, -Wtraditional-conversion and
> -Wlogical-op give false positives, so I believe adding them is not wise.

Here's what I currently use, FYI:

WARNINGS = -g -O -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wendif-labels           \
        -Wformat=2 -Winit-self -Wswitch-enum -Wdeclaration-after-statement  \
        -Wshadow -Wpointer-arith -Wbad-function-cast -Wcast-align           \
        -Wwrite-strings -Wjump-misses-init -Wlogical-op                     \
        -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls          \
        -Wnested-externs -Werror

I do think it's worth fixing code that triggers -Wlogical-op because it's
often unclear.

-Wtraditional-conversion warns about problems when upgrading from K&R to
ANSI C and isn't something we really need to worry about.

> Incidentally, a warning is triggered by -Wpacked in include/inn/dbz.h:

>  typedef struct {
>      char		hash[DBZ_INTERNAL_HASH_SIZE];
>  } PACKED erec;

> Can the PACKED attribute be removed?  I do not understand its usefulness
> for a struct of only one variable.

I left the PACKED on there because I was afraid that removing it might
change the layout of the data structure on disk on some platform, thus
invalidating old history files.  I think it's fairly unlikely that this is
a problem, though.

> -Wold-style-definition returns an issue in innd/icd.c:

>  char *
>  ICDreadactive(endp)
>     char                **endp;
>  {

> Can it be replaced with the following definition?

>  char *
>  ICDreadactive(char **endp)
>  {

Yes, absolutely.

-- 
Russ Allbery (rra at stanford.edu)             <http://www.eyrie.org/~eagle/>

    Please send questions to the list rather than mailing me directly.
     <http://www.eyrie.org/~eagle/faqs/questions.html> explains why.