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

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

Elia Pinto <[email-masked]> 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 <[email-masked]>
> ---
>  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)