Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints

[Jim Meyering]

On Wed, Dec 8, 2021 at 7:48 AM Bruno Haible <bruno@clisp.org> wrote:
>
> > especially given it's in the gcc -Wextra set, not some random flag
>
> Warnings in the '-Wall' category are typically worth eliminating: they
> regularly point to real bugs.
>
> Eliminating '-Wsign-compare' warnings, OTOH, is usually a waste of time, in
> my experience: I have hardly ever found a bug while chasing them.
>
> Robbie Harwood wrote:
> > > As you point out with coreutils, gnulib taking a position like this on
> > > flags has the knock-on effect that, for our grub, we'll need to do one
> > > of the following:
> > >
> > > - Carry a gnulib patch to make the flag work (what we've been doing).
> > > - Change flags in the outer work (i.e., change build options for grub)
> > > - Maintain logic to keep flags gnulib-disliked flags out when building
> > >    the gnulib modules
>
> Paul Eggert replied:
> > Every other Gnulib-using project I know takes the third approach.
>
> So, how about adding the third approach to gnulib-tool?
>
> Rationale:
> Gnulib does not want to dictate their preferred GCC flags to coreutils,
> grub, etc.
> And similarly, we don't want coreutils, grub, etc. to dictate the coding
> style in which gnulib is written.
>
> Implementation idea:
> Since 2021-06-10, gnulib-tool already ensures that the Gnulib modules are
> compiled with '-Wno-error'. This code could be extended to add
> '-Wno-sign-compare' and other warning-disabling options.
>
> Question:
> Which warning options should we disable this way?
> 1) We have some comments in build-aux/gcc-warning.spec.
> 2) When I collect all warnings in the gllib/ directory of a full testdir,
> I get (with gcc-9.3):
>
> $ grep warning: gllib.log | sed -e 's/^.*\[\(-W.*\)\]$/\1/' | LC_ALL=C sort | 
> LC_ALL=C uniq -c
>      72 -Wattribute-warning
>     169 -Wcast-qual
>     457 -Wconversion
>       1 -Wempty-body
>      10 -Wfloat-conversion
>      24 -Wfloat-equal
>       7 -Wimplicit-fallthrough=
>       9 -Wmaybe-uninitialized
>       5 -Wmissing-field-initializers
>       1 -Wpedantic
>       7 -Wredundant-decls
>       8 -Wrestrict
>     156 -Wsign-compare
>    4324 -Wsign-conversion
>       2 -Wtype-limits
>     888 -Wundef
>     139 -Wunsuffixed-float-constants
>       2 -Wunused-function
>     129 -Wunused-parameter
>
> And with gcc-11.2.0:
> $ grep warning: gllib.log | sed -e 's/^.*\[\(-W.*\)\]$/\1/' | LC_ALL=C sort | 
> LC_ALL=C uniq -c
>      72 -Wattribute-warning
>     172 -Wcast-qual
>     509 -Wconversion
>       4 -Wcpp
>       1 -Wempty-body
>       9 -Wfloat-conversion
>      26 -Wfloat-equal
>       7 -Wimplicit-fallthrough=
>       9 -Wmaybe-uninitialized
>       5 -Wmissing-field-initializers
>     432 -Wpedantic
>       7 -Wredundant-decls
>       8 -Wrestrict
>       2 -Wreturn-local-addr
>     168 -Wsign-compare
>    4362 -Wsign-conversion
>       2 -Wtype-limits
>     878 -Wundef
>     155 -Wunsuffixed-float-constants
>       2 -Wunused-function
>     131 -Wunused-parameter
>
> I therefore propose to disable these warnings:
>   -Wattribute-warning
>   -Wcast-qual
>   -Wconversion
>   -Wfloat-conversion
>   -Wfloat-equal
>   -Wimplicit-fallthrough
>   -Wmaybe-uninitialized
>   -Wpedantic
>   -Wrestrict
>   -Wsign-compare
>   -Wsign-conversion
>   -Wtype-limits
>   -Wundef
>   -Wunsuffixed-float-constants
>   -Wunused-function
>   -Wunused-parameter

Hi Bruno,

I don't have terribly strong feelings here, but I would be inclined to
retain some of those.

This exposes low-maint-cost ways to improve APIs, both to help the
compiler help us (e.g., via better optimization or better static
analysis) and to help readers know more at a glance about a labeled
function:
> 72 -Wattribute-warning,

The following are issues that are usually easy/safe to address and
have so few violations that they may be worth addressing or explicitly
suppressing (I haven't looked):
>  7 -Wimplicit-fallthrough=
>  5 -Wmissing-field-initializers

This one is special: historically low-value, but has been known to
catch real bugs. Mark each FP instance to suppress the warning there?
>  9 -Wmaybe-uninitialized