From: Junio C Hamano <gitster@pobox•com>
To: Elia Pinto <gitter.spiros@gmail•com>
Cc: git@vger•kernel.org, jnareb@gmail•com
Subject: Re: [PATCH 2/2] configure.ac: use GIT_CC_CHECK_FLAG_APPEND for adding --with-gcc-warnings configure option
Date: Mon, 03 Nov 2014 11:49:31 -0800 [thread overview]
Message-ID: <xmqq1tpkrrn8.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1415026641-24767-3-git-send-email-gitter.spiros@gmail.com> (Elia Pinto's message of "Mon, 3 Nov 2014 06:57:21 -0800")
Elia Pinto <gitter.spiros@gmail•com> 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 <gitter.spiros@gmail•com>
> ---
> 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.
prev parent reply other threads:[~2014-11-03 19:49 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-03 14:57 [PATCH 0/2] add some new autoconf macros for searching the possible warning flags allowed by the current version of the gcc compiler Elia Pinto
2014-11-03 14:57 ` [PATCH 1/2] configure.ac: add new autoconf macro for checking valid compiler flags Elia Pinto
2014-11-03 19:39 ` Junio C Hamano
2014-11-03 14:57 ` [PATCH 2/2] configure.ac: use GIT_CC_CHECK_FLAG_APPEND for adding --with-gcc-warnings configure option Elia Pinto
2014-11-03 19:49 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq1tpkrrn8.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitter.spiros@gmail$(echo .)com \
--cc=jnareb@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox