Re: warnings in unit tests

[Collin Funk]

Hi Bruno,

On 4/29/24 3:12 PM, Bruno Haible wrote:
> Note that different warning policies may contradict each other. For example,
> some people want to see a warning for
> 
>     int *table = malloc (n * sizeof (int));
> 
> because it has an implicit conversion / "lacks a cast". While other people
> want to see a warning for
> 
>     int *table = (int *) malloc (n * sizeof (int));
> 
> because it has a cast and "casts are dubious". It is impossible to satisfy
> both of these policies at the same time.

Yes, I've seen both in gnulib. I'm pretty sure the cast is required
for C++ (though I think gcc has a warning to make it less strict).

Maybe a 5th category is code taken from another GNU program. Or 4.5th
category since there are only a few glibc files and mini-gmp IIRC.
In that case the original developer and their preferences would have
to be respected of course. :)

> Back to the four sets of code:
> 
> 1) This warning policy is the responsibility of that package's maintainer,
>    obviously.
> 
> 2) These header files are used in compilation units of the package, with
>    CFLAGS or AM_CFLAGS set by the package's maintainer for that package.
>    Therefore in these files we need to avoid even -Wundef, -Wvla, and
>    other kinds of warnings that we wouldn't enable in our code.
> 
> 3) The rest of the lib/ code is under our responsibility, not the
>    responsibility of a package's maintainer. We try to avoid warnings
>    from "reasonable" warning options. More details in the HACKING file.
> 
> 4) The unit tests are also in our responsibility, not the responsibility
>    of a package's maintainer. Here, the primary concern is that is must
>    be *easy* to contribute new unit tests. -Wmissing-variable-declarations
>    warnings _could_ — as Paul wrote — be avoided by adding an 'extern'
>    declaration for each global variable. But this is extra effort that
>    would hinder the addition of new unit tests.

That makes sense to me. Thanks for the explanation.

> Collin, if you want to find relevant findings in the unit tests, by
> using gcc or clang warning options, do *not* use a coreutils build
> for this purpose, but a gnulib testdir instead. (Because the latter
> is not biased by coding style preferences of any package maintainer.)
> 
> Or if you really want to use a coreutils build, first update the
> GL_CFLAG_GNULIB_WARNINGS definition in m4/gnulib-common.m4, so that
> it eliminates useless kinds of warnings.

Ah, thanks for the tip. That sounds quicker than modifying the
Makefiles by hand.

Collin