Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1

2018-03-27 Thread Junio C Hamano
Duy Nguyen  writes:

 On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano  wrote:
>>
>>  - connect.c -Werror=implicit-fallthough around die_initial_contact().
>
> This I did not expect. But I just looked again and I had this option
> explicitly turned off in config.mak! gcc-6.4 and gcc-4.9 do not
> complain about this. gcc-7.3 does. What's your compiler/version?

gcc (Debian 7.3.0-5) 7.3.0


Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1

2018-03-27 Thread Duy Nguyen
On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> The set of extra warnings we enable when DEVELOPER has to be
>> conservative because we can't assume any compiler version the
>> developer may use. Detect the compiler version so we know when it's
>> safe to enable -Wextra and maybe more.
>
> This is a good idea in general, but we are not quite ready without
> some fixups.
>
> Here is a quick summary (not exhaustive) from my trial merge to 'pu'
> (which will be reverted before today's integration is pushed out).
>
>  - json-writer.c triggers -Werror=old-style-decl
>
>  - t/helper/test-json-writer.c triggers Werror=missing-prototypes
>quite a few times.

I expected these (and it was a good reason to push this patch
forward). I think people also reported style problems with this
series.

>
>  - connect.c -Werror=implicit-fallthough around die_initial_contact().
>

This I did not expect. But I just looked again and I had this option
explicitly turned off in config.mak! gcc-6.4 and gcc-4.9 do not
complain about this. gcc-7.3 does. What's your compiler/version?


-- 
Duy


Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1

2018-03-26 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy   writes:

> The set of extra warnings we enable when DEVELOPER has to be
> conservative because we can't assume any compiler version the
> developer may use. Detect the compiler version so we know when it's
> safe to enable -Wextra and maybe more.

This is a good idea in general, but we are not quite ready without
some fixups.  

Here is a quick summary (not exhaustive) from my trial merge to 'pu'
(which will be reverted before today's integration is pushed out).

 - json-writer.c triggers -Werror=old-style-decl

 - t/helper/test-json-writer.c triggers Werror=missing-prototypes
   quite a few times.

 - connect.c -Werror=implicit-fallthough around die_initial_contact().



Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1

2018-03-24 Thread Eric Sunshine
On Sat, Mar 24, 2018 at 8:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> The set of extra warnings we enable when DEVELOPER has to be
> conservative because we can't assume any compiler version the
> developer may use. Detect the compiler version so we know when it's
> safe to enable -Wextra and maybe more.
>
> These warning settings are mostly from my custom config.mak a long
> time ago when I tried to enable as many warnings as possible that can
> still build without showing warnings. Some of them those warnings are

s/them those/those/

> probably worth fixing instead of just suppressing in future.
>
> Helped-by: Jeff King 
> Signed-off-by: Nguyễn Thái Ngọc Duy 


[PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1

2018-03-24 Thread Nguyễn Thái Ngọc Duy
The set of extra warnings we enable when DEVELOPER has to be
conservative because we can't assume any compiler version the
developer may use. Detect the compiler version so we know when it's
safe to enable -Wextra and maybe more.

These warning settings are mostly from my custom config.mak a long
time ago when I tried to enable as many warnings as possible that can
still build without showing warnings. Some of them those warnings are
probably worth fixing instead of just suppressing in future.

Helped-by: Jeff King 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 v2 improves a bit over v1:

 - apple clang support (though I suspect we may want to make it a
   separate compiler family since apple clang has different
   versioning, but I can't and won't work on that front)

 - support CC='ccache gcc' (yes it breaks now if you have spaces in
   your cc path)

 - allow to skip detect-compiler by setting COMPILER_FEATURES in
   config.mak

 I notice Ramsay is working on clean -Wmaybe-uninitialized, if his
 series is merged first I'll stop disabling it here.

 Interdiff

diff --git a/Makefile b/Makefile
index 9dfd152a1e..04b2a39bab 100644
--- a/Makefile
+++ b/Makefile
@@ -434,6 +434,10 @@ all::
 #
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
+#
+# Define DEVELOPER to enable more compiler warnings. Compiler version
+# and faimily are auto detected, but could be overridden by defining
+# COMPILER_FEATURES (see config.mak.dev)
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
diff --git a/config.mak.dev b/config.mak.dev
index 59aef342c4..d8beaf9347 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -8,7 +8,9 @@ CFLAGS += -Wstrict-prototypes
 CFLAGS += -Wunused
 CFLAGS += -Wvla
 
+ifndef COMPILER_FEATURES
 COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
 
 ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
 CFLAGS += -Wtautological-constant-out-of-range-compare
diff --git a/detect-compiler b/detect-compiler
index bc2ea39ef5..70b754481c 100755
--- a/detect-compiler
+++ b/detect-compiler
@@ -9,7 +9,7 @@ CC="$*"
 #
 # FreeBSD clang version 3.4.1 (tags/RELEASE...)
 get_version_line() {
-   "$CC" -v 2>&1 | grep ' version '
+   $CC -v 2>&1 | grep ' version '
 }
 
 get_family() {
@@ -38,12 +38,15 @@ case "$(get_family)" in
 gcc)
print_flags gcc
;;
-*clang)
+clang)
print_flags clang
;;
 "FreeBSD clang")
print_flags clang
;;
+"Apple LLVM")
+   print_flags clang
+   ;;
 *)
: unknown compiler family
;;

 Makefile| 15 +-
 config.mak.dev  | 30 
 detect-compiler | 53 +
 3 files changed, 88 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

diff --git a/Makefile b/Makefile
index a1d8775adb..04b2a39bab 100644
--- a/Makefile
+++ b/Makefile
@@ -434,6 +434,10 @@ all::
 #
 # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
 # which the built Git will run (for instance "x86_64").
+#
+# Define DEVELOPER to enable more compiler warnings. Compiler version
+# and faimily are auto detected, but could be overridden by defining
+# COMPILER_FEATURES (see config.mak.dev)
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -442,15 +446,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-   -Wdeclaration-after-statement \
-   -Wno-format-zero-length \
-   -Wold-style-definition \
-   -Woverflow \
-   -Wpointer-arith \
-   -Wstrict-prototypes \
-   -Wunused \
-   -Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
@@ -1051,7 +1046,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,
diff --git a/config.mak.dev b/config.mak.dev
new file mode 100644
index 00..d8beaf9347
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,30 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+ifndef COMPILER_FEATURES
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter 
clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+CFLAGS += -Wmissing-prototypes
+CFLAGS += -Wno-empty-body
+CF