Re: more compile warnings with new factor

[Pádraig Brady]

On 10/23/2012 08:46 AM, Jim Meyering wrote:
> Torbjorn Granlund wrote:
> > Pádraig Brady <address@hidden> writes:
> >
> >    If I manually undef HAVE_GMP at the top of factor.c, then...
> >
> >    $ make src/factor.o
> >    src/factor.c:154:0: error: macro "__GMP_GNUC_PREREQ" is not used
> > [-Werror=unused-macros]
> >
> > It is used by some configs in longlong.h.  It cannot be removed unless
> > you fork longlong.h.
>
> Hi Torbjörn,
>
> FYI, I made this small change to longlong.h a month ago:
>
>      build: avoid warning about unused macro
>      ...
>      * src/longlong.h (__GMP_DECLSPEC): Define if not already defined.
>
> +#ifndef __GMP_DECLSPEC
> +#define __GMP_DECLSPEC /* empty */
> +#endif
> +
>
> ...
> >    src/factor.c:2146:19: error: variable '_p0' set but not used
> > [-Werror=unused-but-set-variable]
> >
> >    The above point to redundant processing in umul_ppmm()
> >    (or at least our use of it).
> >
> > The code sometimes needs the upper half of a product, but umul_ppmm
> > generates the full product as two halves.  You could insert bogus
> > dependencies on the low half, most likely making generated code worse.
> > Or remove the bogus warning.

I'll remove the warnings without impacting code quality.

> Pádraig,
> If you don't find a cleaner solution, I wouldn't mind marking those
> variables with ATTRIBUTE_UNUSED when HAVE_GMP is not defined.  E.g.,
>
>      #if HAVE_GMP
>      # define UNUSED_WITH_NO_GMP /* empty */
>      #else
>      # define UNUSED_WITH_NO_GMP ATTRIBUTE_UNUSED
>      #endif
>
> Alternatively, and cleaner still, disable the -Wunused-but-set-variable
> option when configure detects no GMP support.  These days, building
> coreutils without GMP support seems less and less relevant.
>
> Finally, the simplest option of all would be fine, too: do nothing,
> and let those who configure with --enable-gcc-warnings figure out
> that they can get past this by building with "make WERROR_CFLAGS=".
>
> >    src/factor.c: At top level:
> >    src/factor.c:151:0: error: macro "ASSERT" is not used [-Werror=unused-macros]
> >    src/factor.c:154:0: error: macro "__GMP_GNUC_PREREQ" is not used
> > [-Werror=unused-macros]
> >
> > I fail to understand how anybody can regard an unused macro as an
> > *error*.
>
> When configured with gcc and --enable-gcc-warnings we enable -Werror,
> to ensure that no warnings slip by unnoticed.
> That is not the default, though I'm tempted to make it
> the default when building from git.

Already the case since 47fd706, hence why a little more important to clean up.

> Yes, we have high standards.
>
> >    I'll clean up these tomorrow.
> >
> > Aren't this Draconian options ("ask the compiler to nag about everything
> > it can, then treat nagging as errors in the code") self-defeating?  You
> > need to avoid natural ways of writing things, and instead go for
> > contorted solutions.  I spent a few hours trying to satisfy your demands
> > in this area, and I am sure you maintainers have spent a lot of time on
> > it.
>
> We've learned that while many of the warnings are
> not about real bugs, some do indicate real problems.
> An unused macro or variable could well indicate a typo
> in the definition/declaration or in all uses.
>
> > I for one prefer to spend my time on improving GNU, not define an
> > artifical goal like this one, and work on satifying that.
>
> We have moved to using very strict warning settings gradually over the
> years, carefully disabling the few warning options for which the cost
> consistently outweighs the benefit.
>
> We really do believe that what we are doing is worthwhile.

The onus is on those who know the code (now),
to minimize warnings to ease maintenance down the line.

thanks,
Pádraig.