[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/6] Use gnulib's manywarnings & warnings modules



On 04/04/2011 10:19 AM, Daniel P. Berrange wrote:
> Remove custom code for checking compiler warnings, using
> gl_WARN_ADD instead. Don't list all flags ourselves, use
> gnulib's gl_MANYWARN_ALL_GCC to get all possible GCC flags,
> then turn off the ones we don't want yet.
> 
> * acinclude.m4: Rewrite to use gl_WARN_ADD and gl_MANYWARN_ALL_GCC
> * bootstrap.conf: Add warnings & manywarnings
> * configure.ac: Switch to gl_WARN_ADD
> * m4/compiler-flags.m4: Obsoleted by gl_WARN_ADD
> ---
>  acinclude.m4         |  158 +++++++++++++++++++++++++++-----------------------
>  bootstrap.conf       |    2 +
>  configure.ac         |   15 +++--
>  m4/compiler-flags.m4 |   48 ---------------
>  4 files changed, 96 insertions(+), 127 deletions(-)
>  delete mode 100644 m4/compiler-flags.m4

> +
> +        # List of warnings that are not relevant / wanted
> +        dontwarn="$dontwarn -Wc++-compat"             # Don't care about C++ compiler compat
> +        dontwarn="$dontwarn -Wtraditional"            # Don't care about ancient C standard compat
> +        dontwarn="$dontwarn -Wtraditional-conversion" # Don't care about ancient C standard compat
> +        dontwarn="$dontwarn -Wsystem-headers"         # Ignore warnings in /usr/include
> +        dontwarn="$dontwarn -Wpadded"                 # Happy for compiler to add struct padding
> +        dontwarn="$dontwarn -Wunreachable-code"       # GCC very confused with -O2
> +        dontwarn="$dontwarn -Wconversion"             # Too many to deal with
> +        dontwarn="$dontwarn -Wsign-conversion"        # Too many to deal with
> +        dontwarn="$dontwarn -Wvla"                    # GNULIB gettext.h violates
> +        dontwarn="$dontwarn -Wundef"                  # Many GNULIB violations
> +        dontwarn="$dontwarn -Wcast-qual"              # Need to allow bad cast for execve()
> +        dontwarn="$dontwarn -Wlong-long"              # We need to use long long in many places
> +        dontwarn="$dontwarn -Wswitch-default"         # We allow manual list of all enum cases without default:
> +        dontwarn="$dontwarn -Wswitch-enum"            # We allow optional default: instead of listing all enum values
> +        dontwarn="$dontwarn -Wstrict-overflow"        # Not a problem since we don't use -fstrict-overflow
> +        dontwarn="$dontwarn -Wunsafe-loop-optimizations" # Not a problem since we don't use -funsafe-loop-optimizations

Seems okay (but long lines - any way to break that into 80 columns?)

> +
> +        # We might fundamentally need some of these disabled forever, but ideally
> +        # we'd turn many of them on
> +        dontwarn="$dontwarn -Wformat-nonliteral"

This one may need to always be disabled, thanks to virAsprintf (it would
be nicer if we could disable for just one or two files, rather than
globally).

> +        dontwarn="$dontwarn -Wfloat-equal"
> +        dontwarn="$dontwarn -Wdeclaration-after-statement"

If gcc would do better, I'd love to guarantee C99 and take advantage of
this (smaller scopes have maintenance benefits) - thus, I envision this
one being permanent (float it up to the earlier set).

> +        dontwarn="$dontwarn -Wcast-qual"
> +        dontwarn="$dontwarn -Wconversion"
> +        dontwarn="$dontwarn -Wsign-conversion"
> +        dontwarn="$dontwarn -Wold-style-definition"
> +        dontwarn="$dontwarn -Wmissing-noreturn"
> +        dontwarn="$dontwarn -Wpacked"
> +        dontwarn="$dontwarn -Wunused-macros"
> +        dontwarn="$dontwarn -Woverlength-strings"
> +        dontwarn="$dontwarn -Wmissing-format-attribute"
> +        dontwarn="$dontwarn -Wstack-protector"

Yes, I agree that the rest of these should be temporary, and are
probably worth fixing.

> +
> +        # Get all possible GCC warnings
> +        gl_MANYWARN_ALL_GCC([maybewarn])
> +
> +        # Remove the ones we don't want, blacklisted earlier
> +        gl_MANYWARN_COMPLEMENT([wantwarn], [$maybewarn], [$dontwarn])
> +
> +        # Check for $CC support of each warning
> +        for w in $wantwarn; do
> +          gl_WARN_ADD([$w])
> +        done
> +
> +        # GNULIB uses '-W' which includes a bunch of stuff,
> +        # kinda like -Wextra. Unfortunately, it means you

"kinda like"? Exactly like!  -W was the old spelling, -Wextra was the
new one (added quite some time ago; something like gcc 3.4, off the top
of my head).

> +        # can't simply use '-Wsign-compare' with gl_MANYWARN_COMPLEMENT
> +        # So we have -W enabled, and then have to explicitly turn off
> +        gl_WARN_ADD(-Wno-sign-compare)
> +
> +        # This should be < 256 really, but with PATH_MAX everywhere
> +        # we have doom, even with 4096. In fact we have some functions
> +        # with several PATH_MAX sized variables :-( We should kill off
> +        # all PATH_MAX usage and then lower this limit
> +        gl_WARN_ADD([-Wframe-larger-than=65700])
> +        dnl gl_WARN_ADD([-Wframe-larger-than=4096])
> +        dnl gl_WARN_ADD([-Wframe-larger-than=256])
> +
> +        # Extra special flags
> +        gl_WARN_ADD([-Wp,-D_FORTIFY_SOURCE=2])
> +        dnl Fedora only uses -fstack-protector, but doesn't seem to
> +        dnl be great overhead in adding -fstack-protector-all instead
> +        dnl gl_WARN_ADD([-fstack-protector])
> +        gl_WARN_ADD([-fstack-protector-all])
> +        gl_WARN_ADD([--param=ssp-buffer-size=4])
> +        gl_WARN_ADD([-fexceptions])
> +        gl_WARN_ADD([-fasynchronous-unwind-tables])
> +        gl_WARN_ADD([-fdiagnostics-show-option])
> +
> +        if test "$enable_compile_warnings" = "error"
> +        then
> +          gl_WARN_ADD([-Werror])
> +        fi

Looks like a good conversion.

> @@ -81,6 +82,7 @@ timegm
>  uname
>  useless-if-before-free
>  usleep
> +warnings
>  vasprintf
>  verify
>  vc-list-files

Where did you learn to sort?  :)

> diff --git a/configure.ac b/configure.ac
> index a8d223a..7874c30 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1970,12 +1970,14 @@ AC_ARG_ENABLE([test-coverage],
>  enable_coverage=$enableval
>  
>  if test "${enable_coverage}" = yes; then
> -  COMPILER_FLAGS=
> -  gl_COMPILER_FLAGS(-fprofile-arcs)
> -  gl_COMPILER_FLAGS(-ftest-coverage)
> -  AC_SUBST([COVERAGE_CFLAGS], [$COMPILER_FLAGS])
> -  AC_SUBST([COVERAGE_LDFLAGS], [$COMPILER_FLAGS])
> -  COMPILER_FLAGS=
> +  save_WARN_CFLAGS=$WARN_CFLAGS
> +  WARN_CFLAGS=
> +  gl_WARN_ADD(-fprofile-arcs)
> +  gl_WARN_ADD(-ftest-coverage)

Nit: use consistent m4 quoting: gl_WARN_ADD([-fprofile-arcs])

ACK with nits addressed.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]