[PATCH 0/2] add some new autoconf macros for searching the possible warning flags allowed by the current version of the gcc compiler

[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]

This patch series introduces some new autoconf macros for searching the possible warning flags allowed by the current version of the gcc compiler. 
Using these macros it adds a new option --enable-gcc-warnings (default off). There are several possible gcc warnings flags, 
i have included some of the most useful in my opinion but certainly there are several other perhaps more useful for git, so comments are welcome. 
Naturally the new macro are not limited only to the portable search of the gcc warning flags or to gcc in first place in any way

Elia Pinto (2):
  configure.ac: add new autoconf macro for checking valid compiler
    flags
  configure.ac: use GIT_CC_CHECK_FLAG_APPEND for adding
    --with-gcc-warnings configure option

 Makefile     |   12 ++++-
 configure.ac |  141 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 149 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] configure.ac: add new autoconf macro for checking valid compiler flags

[Elia Pinto]

Add GIT_CC_CHECK_FLAG_APPEND, GIT_CC_CHECK_FLAGS_APPEND
and GIT_CC_CHECK_LDFLAGS autoconf macro.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 Makefile     |    6 ++++--
 configure.ac |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 827006b..23485f1 100644
--- a/Makefile
+++ b/Makefile
@@ -344,10 +344,12 @@ 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
-LDFLAGS =
+CFLAGS = -g -O2 -Wall $(GIT_CFLAGS)
+LDFLAGS = $(GIT_LDFLAGS)
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 STRIP ?= strip
diff --git a/configure.ac b/configure.ac
index 6af9647..c67ca60 100644
--- a/configure.ac
+++ b/configure.ac
@@ -139,6 +139,51 @@ if test -n "$1"; then
 fi
 ])
 
+
+dnl Check if FLAG in ENV-VAR is supported by compiler and append it
+dnl to WHERE-TO-APPEND variable
+dnl GIT_CC_CHECK_FLAG_APPEND([WHERE-TO-APPEND], [ENV-VAR], [FLAG])
+
+AC_DEFUN([GIT_CC_CHECK_FLAG_APPEND], [
+  AC_CACHE_CHECK([if $CC supports flag $3 in envvar $2],
+                 AS_TR_SH([cc_cv_$2_$3]),
+          [eval "AS_TR_SH([cc_save_$2])='${$2}'"
+           eval "AS_TR_SH([$2])='-Werror $3'"
+           AC_LINK_IFELSE([AC_LANG_SOURCE([int a = 0; int main(void) { return a; } ])],
+                          [eval "AS_TR_SH([cc_cv_$2_$3])='yes'"],
+                          [eval "AS_TR_SH([cc_cv_$2_$3])='no'"])
+           eval "AS_TR_SH([$2])='$cc_save_$2'"])
+
+  AS_IF([eval test x$]AS_TR_SH([cc_cv_$2_$3])[ = xyes],
+        [eval "$1='${$1} $3'"])
+])
+
+dnl GIT_CC_CHECK_FLAGS_APPEND([WHERE-TO-APPEND], [ENV-VAR], [FLAG1 FLAG2])
+AC_DEFUN([GIT_CC_CHECK_FLAGS_APPEND], [
+  for flag in $3; do
+    GIT_CC_CHECK_FLAG_APPEND($1, $2, $flag)
+  done
+])
+
+dnl Check if the flag is supported by linker (cacheable)
+dnl GIT_CC_CHECK_LDFLAGS([FLAG], [ACTION-IF-FOUND],[ACTION-IF-NOT-FOUND])
+
+AC_DEFUN([GIT_CC_CHECK_LDFLAGS], [
+  AC_CACHE_CHECK([if $CC supports $1 flag],
+    AS_TR_SH([cc_cv_ldflags_$1]),
+    [ac_save_LDFLAGS="$LDFLAGS"
+     LDFLAGS="$LDFLAGS $1"
+     AC_LINK_IFELSE([int main() { return 1; }],
+       [eval "AS_TR_SH([cc_cv_ldflags_$1])='yes'"],
+       [eval "AS_TR_SH([cc_cv_ldflags_$1])="])
+     LDFLAGS="$ac_save_LDFLAGS"
+    ])
+
+  AS_IF([eval test x$]AS_TR_SH([cc_cv_ldflags_$1])[ = xyes],
+    [$2], [$3])
+])
+
+
 ## Configure body starts here.
 
 AC_PREREQ(2.59)
-- 
1.7.10.4

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

[Elia Pinto]

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)
@@ -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.
-- 
1.7.10.4

Re: [PATCH 1/2] configure.ac: add new autoconf macro for checking valid compiler flags

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

> Add GIT_CC_CHECK_FLAG_APPEND, GIT_CC_CHECK_FLAGS_APPEND
> and GIT_CC_CHECK_LDFLAGS autoconf macro.

... which does what and for what purpose?

Don't explain it to me in your response, or tell me to read the
patch text.  I am speaking for those who have to read "git log"
output down the line and need to get the big picture.

In the bigger picture, what is important is "why" than "how".

You add a handy way to give list of possible compilation flags and
add as many of them as we can without triggering errors to CFLAGS
and LDFLAGS.  Why is that a good thing to do?  What kind of
compilation flags you are planning to throw at these macros?  I
think a mere mention of the latter will be sufficient to remind the
reader why this change would be a good idea.

>
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
>  Makefile     |    6 ++++--
>  configure.ac |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 827006b..23485f1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -344,10 +344,12 @@ 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
> -LDFLAGS =
> +CFLAGS = -g -O2 -Wall $(GIT_CFLAGS)
> +LDFLAGS = $(GIT_LDFLAGS)
>  ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>  ALL_LDFLAGS = $(LDFLAGS)
>  STRIP ?= strip
> diff --git a/configure.ac b/configure.ac
> index 6af9647..c67ca60 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -139,6 +139,51 @@ if test -n "$1"; then
>  fi
>  ])
>  
> +
> +dnl Check if FLAG in ENV-VAR is supported by compiler and append it
> +dnl to WHERE-TO-APPEND variable
> +dnl GIT_CC_CHECK_FLAG_APPEND([WHERE-TO-APPEND], [ENV-VAR], [FLAG])
> +
> +AC_DEFUN([GIT_CC_CHECK_FLAG_APPEND], [
> +  AC_CACHE_CHECK([if $CC supports flag $3 in envvar $2],
> +                 AS_TR_SH([cc_cv_$2_$3]),
> +          [eval "AS_TR_SH([cc_save_$2])='${$2}'"
> +           eval "AS_TR_SH([$2])='-Werror $3'"
> +           AC_LINK_IFELSE([AC_LANG_SOURCE([int a = 0; int main(void) { return a; } ])],
> +                          [eval "AS_TR_SH([cc_cv_$2_$3])='yes'"],
> +                          [eval "AS_TR_SH([cc_cv_$2_$3])='no'"])
> +           eval "AS_TR_SH([$2])='$cc_save_$2'"])
> +
> +  AS_IF([eval test x$]AS_TR_SH([cc_cv_$2_$3])[ = xyes],
> +        [eval "$1='${$1} $3'"])
> +])
> +
> +dnl GIT_CC_CHECK_FLAGS_APPEND([WHERE-TO-APPEND], [ENV-VAR], [FLAG1 FLAG2])
> +AC_DEFUN([GIT_CC_CHECK_FLAGS_APPEND], [
> +  for flag in $3; do
> +    GIT_CC_CHECK_FLAG_APPEND($1, $2, $flag)
> +  done
> +])
> +
> +dnl Check if the flag is supported by linker (cacheable)
> +dnl GIT_CC_CHECK_LDFLAGS([FLAG], [ACTION-IF-FOUND],[ACTION-IF-NOT-FOUND])

Missing SP after comma?  I do not particularly mind commas without
leading SP in m4 source, but please be consistent unless there is a
compelling reason not to (and I do not think there is in this case).

> +
> +AC_DEFUN([GIT_CC_CHECK_LDFLAGS], [
> +  AC_CACHE_CHECK([if $CC supports $1 flag],
> +    AS_TR_SH([cc_cv_ldflags_$1]),
> +    [ac_save_LDFLAGS="$LDFLAGS"
> +     LDFLAGS="$LDFLAGS $1"
> +     AC_LINK_IFELSE([int main() { return 1; }],
> +       [eval "AS_TR_SH([cc_cv_ldflags_$1])='yes'"],
> +       [eval "AS_TR_SH([cc_cv_ldflags_$1])="])
> +     LDFLAGS="$ac_save_LDFLAGS"
> +    ])
> +
> +  AS_IF([eval test x$]AS_TR_SH([cc_cv_ldflags_$1])[ = xyes],
> +    [$2], [$3])
> +])
> +
> +
>  ## Configure body starts here.
>  
>  AC_PREREQ(2.59)

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

[Junio C Hamano]

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.