Re: [PATCH 3/3] Makefile: use -Wdeclaration-after-statement if supported

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 17, 2012, 01:52 UTC
Message-ID: <7vk3shphru.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1355686561-1057-4-git-send-email-git@adamspiers.org>

Adam Spiers <git@adamspiers.org> writes:

Show 18 quoted lines

> If we adopt this approach,...
> diff --git a/Makefile b/Makefile
> index a49d1db..aae70d4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -331,8 +331,13 @@ endif
>  # CFLAGS and LDFLAGS are for the users to override from the command line.
>  
>  CFLAGS = -g -O2 -Wall
> +GCC_DECL_AFTER_STATEMENT = \
> +	$(shell $(CC) --help -v 2>&1 | \
> +		grep -q -- -Wdeclaration-after-statement && \
> +	  echo -Wdeclaration-after-statement)
> +GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)
> +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)
>  LDFLAGS =
> -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>  ALL_LDFLAGS = $(LDFLAGS)

Please do not do this.

People cannot disable it from the command line, like:

    $ make V=1 CFLAGS='-g -O0 -Wall'

If anything, this should be part of the default CFLAGS.

More importantly, this will run the $(shell ...) struct once for every *.o file we produce, I think, in addition to running it twice for the whole build. If you add this:

@@ -345,7 +345,8 @@ CFLAGS = -g -O2 -Wall
 GCC_DECL_AFTER_STATEMENT = \
 	$(shell $(CC) --help -v 2>&1 | \
 		grep -q -- -Wdeclaration-after-statement && \
-	  echo -Wdeclaration-after-statement)
+	  echo -Wdeclaration-after-statement; \
+	  echo >&2 GCC_DECL_AFTER_STATEMENT CRUFT)
 GCC_FLAGS = $(GCC_DECL_AFTER_STATEMENT)
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) $(GCC_FLAGS)
 LDFLAGS =

remove git.o and dir.o from a fully built tree, and then try to
rebuild these two files, you will get this:

    $ make V=1 git.o dir.o
    GCC_DECL_AFTER_STATEMENT CRUFT
    GCC_DECL_AFTER_STATEMENT CRUFT
    GCC_DECL_AFTER_STATEMENT CRUFT
    cc -o git.o -c -MF ./.depend/git.o.d -MMD -MP  -g -O2 -Wall \
    -Wdeclaration-after-statement -I.  -DHAVE_PATHS_H -DHAVE_DEV_TTY \
    -DXDL_FAST_HASH -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY \
    -DNO_MKSTEMPS -DSHELL_PATH='"/bin/sh"' \
    '-DGIT_HTML_PATH="share/doc/git-doc"' '-DGIT_MAN_PATH="share/man"' \
    '-DGIT_INFO_PATH="share/info"' git.c
    GCC_DECL_AFTER_STATEMENT CRUFT
    cc -o dir.o -c -MF ./.depend/dir.o.d -MMD -MP  -g -O2 -Wall \
    -Wdeclaration-after-statement -I.  -DHAVE_PATHS_H -DHAVE_DEV_TTY \
    -DXDL_FAST_HASH -DSHA1_HEADER='<openssl/sha.h>'  -DNO_STRLCPY \
    -DNO_MKSTEMPS -DSHELL_PATH='"/bin/sh"'  dir.c
    $ make V=1 git.o dir.o
    GCC_DECL_AFTER_STATEMENT CRUFT
    GCC_DECL_AFTER_STATEMENT CRUFT
    make: `dir.o' is up to date.