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

Adam Spiers [off-list ref] writes:

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.

On Sun, Dec 16, 2012 at 05:52:05PM -0800, Junio C Hamano wrote:

Adam Spiers [off-list ref] writes:

quoted

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.

[snipped]

OK; I expect these issues with the implementation are all
surmountable.  I did not necessarily expect this to be the final
implementation anyhow, as indicated by my comments below the divider
line.  However it's not clear to me what you think about the idea in
principle, and whether other compiler flags would merit inclusion.

(And also, please don't let this discussion hold up acceptance of the
two prior patches in the series.  Even though they are independent,
they are somewhat logically related so I grouped them into the same
series, although I'm not sure if that was the right thing to do.)