Re: Removing -Wunsuffixed-float-constants, -Wdouble-promotion, -Wformat-zero-length

[Simon Josefsson]

Eric Blake <address@hidden> writes:

> On 11/30/2011 03:20 AM, Simon Josefsson wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> On 11/29/2011 02:46 PM, Eric Blake wrote:
>>>>> Unless there are objections (portability?)
>>>>
>>>> Aargh.  I just reread C99.
>>>>
>>>> F (and f) for float, and L (or l) for long double are required, but D
>>>> (or d) for double is a GNU extension.
>>>>
>>>> Since we can't silence the warning without adding an explicit 'D', but
>>>> 'D' is not standardized, I have changed my mind.  Let's nuke the warning.
>>>
>>> So for starters, I'm pushing this:
>> 
>> Please revert or change this -- like other warnings, this warning is
>> useful in some projects and not useful in others, and the idea with
>> manywarnings.m4 is that all warnings should be enabled and each project
>> has to disable the warnings they don't like incrementally.  See
>> doc/manywarnings.texi for background.
>
> We already concluded that the warning is bogus as a reasonable warning -
> there is no way to write portable code that both complies with C99 and
> silences the warning without either ditching 'double' (using only
> 'float' and 'long double' constants) or using non-standard extensions
> (gcc's '1.0D' for double).

The warning can still be useful -- it would be triggered when I
introduce code that has this property into my projects, and I will be
forced to think about whether the code may be rewritten in another style
or silence the warning.  I much prefer this from not knowing that I
introduced code with that behaviour.

As was indicated earlier in this thread, when reviewing code that
triggered this warning, some code was rewritten to not use floats at all
which is one example of a good consequence of using the warning flag.

> But I can see your counterargument of having a complete list of all
> possible gcc warnings, followed by a second list that filters out
> warnings that make no sense in a gnulib project, as well as lists of
> warnings that make no sense in a C-only, a C++-only, and a dual C/C++
> program.  For example, -Wc++-compat makes no sense for a C-only program,
> but makes perfect sense for a dual C/C++ program.  Libtool is an example
> of a dual program, that strives to use the subset of C and C++ so that
> it can be compiled by either language for use in both C and C++ programs.
>
> If we have various filter lists, for all warnings minus those not suited
> for this typical usage pattern, as well as a way to easily select which
> filters to apply, that would indeed be nicer.
> -Wunsuffixed-float-constants should then appear on the list of all
> warnings, but also on pretty much every filter list as being worthless
> for portable code.  We could also have a filter list for warnings that
> will not work with gnulib code directly, but which projects may still
> wish to use on their own code (see how coreutils has split warning
> levels, for example).

This sounds good to me.  We'd might as well improve the rather ugly code
that people have to put into their configure.ac's in order to use the
manywarnings module, see below...

>> I'm fine with creating another function gl_MANYWARN_ALL_REASONEBLE_GCC
>> or similar if you prefer that, but let's keep the base function clean
>> from subjective filtering of warning flags.  It should just generate a
>> list of all warning flags that can possibly be enabled in gcc.
>
> Maybe more like gl_MANYWARN_ALL_GCC_EXCEPT(VARIABLE, [EXCLUSION]), where
> it would be called like:
>
> gl_MANYWARN_ALL_GCC_EXCEPT([WARN_CFLAGS],
>   [gl_MANYWARN_NO_C++_WARNINGS])
> gl_MANYWARN_ALL_GCC_EXCEPT([GNULIB_WARN_CFLAGS],
>   [gl_MANYWARN_NO_C++_WARNINGS], [gl_MANYWARN_NO_GNULIB_WARNINGS])

Yes.  The current libidn configure.ac code is this:

if test "$gl_gcc_warnings" = yes; then
  gl_WARN_ADD([-Werror], [WERROR_CFLAGS])
  gl_WARN_ADD([-Wframe-larger-than=112], [WSTACK_CFLAGS])

  nw="$nw -Wsystem-headers"         # Don't let system headers trigger warnings
  nw="$nw -Wpadded"                 # Struct in src/idn_cmd.h is not padded
  nw="$nw -Wformat"                 # Self tests and examples print size_t as %d
  nw="$nw -Wc++-compat"             # We don't care strongly about C++ compilers
  nw="$nw -Woverlength-strings"     # Some of our strings are too large
  nw="$nw -Wsign-conversion"        # Too many warnings for now
  nw="$nw -Wconversion"             # Too many warnings for now
  nw="$nw -Wtraditional"            # Warns on #elif which we use often
  nw="$nw -Wtraditional-conversion" # Too many warnings for now
  nw="$nw -Wmissing-noreturn"       # Too many warnings for now
  nw="$nw -Wunreachable-code"       # Too many false positives
  nw="$nw -Wlogical-op"             # Too many false positives
  nw="$nw -Wsuggest-attribute=pure" # Is it worth using attributes?
  nw="$nw -Wsuggest-attribute=const" # Is it worth using attributes?

  gl_MANYWARN_ALL_GCC([ws])
  gl_MANYWARN_COMPLEMENT(ws, [$ws], [$nw])
  for w in $ws; do
    gl_WARN_ADD([$w])
  done

  gl_WARN_ADD([-fdiagnostics-show-option])
fi

Possibly that could be rewritten into:

if test "$gl_gcc_warnings" = yes; then
  gl_WARN_ADD([-Werror], [WERROR_CFLAGS])
  gl_WARN_ADD([-Wframe-larger-than=112], [WSTACK_CFLAGS])
  gl_MANYWARN_ALL_GCC_EXCEPT([WARN_CFLAGS],
    [-Wsystem-headers -Wpadded -Wformat -Wc++-compat
    -Woverlength-strings -Wsign-conversion -Wconversion -Wtraditional
    -Wtraditional-conversion -Wmissing-noreturn -Wunreachable-code
    -Wlogical-op -Wsuggest-attribute=pure -Wsuggest-attribute=const])
  gl_WARN_ADD([-fdiagnostics-show-option])
fi

/Simon