Subject: Re: [PATCH 2/2] configure.ac: use GIT_CC_CHECK_FLAG_APPEND for adding --with-gcc-warnings configure option

From: Junio C Hamano <[email-masked]>
Date: Mon, 03 Nov 2014 11:49:31 -0800

Elia Pinto <[email-masked]> writes:

> Use the GIT_CC_CHECK_FLAGS_APPEND autoconf macro
> for add in a portable way the new configure option
> --enable-gcc-warnings (default off).
>
> Signed-off-by: Elia Pinto <[email-masked]>
> ---
>  Makefile     |   12 ++++++--
>  configure.ac |   96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 103 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 23485f1..9db34e2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -344,11 +344,9 @@ GIT-VERSION-FILE: FORCE
>  	@$(SHELL_PATH) ./GIT-VERSION-GEN
>  -include GIT-VERSION-FILE
>  
> -GIT_CFLAGS  =
> -GIT_LDFLAGS =
>  # CFLAGS and LDFLAGS are for the users to override from the command line.
>  
> -CFLAGS = -g -O2 -Wall $(GIT_CFLAGS)
> +CFLAGS = -g -O2 -Wall 
>  LDFLAGS = $(GIT_LDFLAGS)
>  ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>  ALL_LDFLAGS = $(LDFLAGS)

Reverting what you did just a minute ago?

I am debating myself if I should say that the new placement may make
more sense.  On the one hand, -Wanything is more or less developer's
personal taste and should be easily overridable via CFLAGS for each
invocation of "make", just like -g and -O2.  On the other hand, some
class of error checking is so useful and bulletproof not to give us
false positives that we may want to encourage people to always use
them when available.  The placement in 1/2 was in line with the
former, but the updated placement makes it very hard to selectively
disable GCC crud that barfs unnecessarily without disabling
everything by GIT_CFLAGS="", I am afraid.

Looking at the way the patch to configure.ac tries to add many
things, I am not sure if these two patches are good idea (note: I
didn't say "I am sure this is going in a wrong direction").  Is it
adding everything under the sun, or after careful consideration on
each and every ones to see how false-positive-prone it is?

> @@ -1517,6 +1515,14 @@ ifdef DEFAULT_HELP_FORMAT
>  BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
>  endif
>  
> +ifdef GIT_CFLAGS
> +BASIC_CFLAGS += $(GIT_CFLAGS)
> +endif
> +
> +ifdef GIT_LDFLAGS
> +BASIC_LDFLAGS += $(GIT_LDFLAGS)
> +endif
> +
>  ALL_CFLAGS += $(BASIC_CFLAGS)
>  ALL_LDFLAGS += $(BASIC_LDFLAGS)
>  
> diff --git a/configure.ac b/configure.ac
> index c67ca60..95d5d10 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1,4 +1,4 @@
> -#                                               -*- Autoconf -*-
> +#                                               -*- Autoconf -*- \
>  # Process this file with autoconf to produce a configure script.
>  
>  ## Definitions of private macros.
> @@ -433,7 +433,99 @@ AS_HELP_STRING([],[ARG is the full path to the Tcl/Tk interpreter.])
>  AS_HELP_STRING([],[Bare --with-tcltk will make the GUI part only if])
>  AS_HELP_STRING([],[Tcl/Tk interpreter will be found in a system.]),
>  GIT_PARSE_WITH(tcltk))
> -#
> +
> +
> +# Turn gcc warning
> +
> +AC_ARG_ENABLE([gcc-warnings],
> +  [AS_HELP_STRING([--enable-gcc-warnings],
> +                  [turn on GCC warnings (for developers)@<:@default=no@:>@])],
> +  [case $enableval in
> +     yes|no) ;;
> +     *)      AC_MSG_ERROR([bad value $enableval for gcc-warnings option]) ;;
> +   esac
> +   git_gcc_warnings=$enableval],
> +  [git_gcc_warnings=no]
> +)
> +
> +AS_IF([test "x$git_gcc_warnings" = xyes ],[ 
> +# Add/Delete as needed
> +GIT_CC_CHECK_FLAGS_APPEND([with_cflags], [CFLAGS], [\
> +  -Wall  \
> +  -Wextra  \
> +  -Wformat-y2k  \
> +  -fdiagnostics-show-option \
> +  -funit-at-a-time \
> +  -fstrict-aliasing \
> +  -Wstrict-overflow \
> +  -fstrict-overflow \
> +  -Wpointer-arith \
> +  -Wundef \
> +  -Wformat-security \
> +  -Winit-self \
> +  -Wmissing-include-dirs \
> +  -Wunused \
> +  -Wunknown-pragmas \
> +  -Wstrict-aliasing \
> +  -Wshadow \
> +  -Wbad-function-cast \
> +  -Wcast-align \
> +  -Wwrite-strings \
> +  -Wlogical-op \
> +  -Waggregate-return \
> +  -Wstrict-prototypes \
> +  -Wold-style-definition \
> +  -Wmissing-prototypes \
> +  -Wmissing-declarations \
> +  -Wmissing-noreturn \
> +  -Wmissing-format-attribute \
> +  -Wredundant-decls \
> +  -Wnested-externs \
> +  -Winline \
> +  -Winvalid-pch \
> +  -Wvolatile-register-var \
> +  -Wdisabled-optimization \
> +  -Wbuiltin-macro-redefined \
> +  -Wmudflap \
> +  -Wpacked-bitfield-compat \
> +  -Wsync-nand \
> +  -Wattributes \
> +  -Wcoverage-mismatch \
> +  -Wmultichar \
> +  -Wcpp \
> +  -Wdeprecated-declarations \
> +  -Wdiv-by-zero \
> +  -Wdouble-promotion \
> +  -Wendif-labels \
> +  -Wformat-contains-nul \
> +  -Wformat-extra-args \
> +  -Wformat-zero-length \
> +  -Wformat=2 \
> +  -Wmultichar \
> +  -Wnormalized=nfc \
> +  -Woverflow \
> +  -Wpointer-to-int-cast \
> +  -Wpragmas \
> +  -Wsuggest-attribute=const \
> +  -Wsuggest-attribute=noreturn \
> +  -Wsuggest-attribute=pure \
> +  -Wtrampolines \
> +  -Wno-missing-field-initializers \
> +  -Wno-sign-compare \
> +  -Wjump-misses-init \
> +  -Wno-format-nonliteral \
> +  -fstack-protector-all \
> +  -fasynchronous-unwind-tables \
> +  -fdiagnostics-show-option \
> +  -funit-at-a-time \
> +  -fipa-pure-const \
> +  -Wno-aggregate-return \
> +  -Wno-redundant-decls \
> +  -Wdeclaration-after-statement ])
> +
> +GIT_CONF_SUBST([GIT_CFLAGS],[$with_cflags])
> +])
> +
>  
>  
>  ## Checks for programs.