[PATCH] travis-ci: enable more warnings on travis linux-gcc job

[PATCH] travis-ci: enable more warnings on travis linux-gcc job

[Nguyễn Thái Ngọc Duy]

We have DEVELOPER config to enable more warnings, but since we can't set
a fixed gcc version to all devs, that has to be a bit more conservative.
On travis, we know almost exact version to be used (bumped to 6.x for
more warnings), we could be more aggressive.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 .travis.yml     |  3 +++
 ci/run-build.sh | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 4684b3f4f3..273b1d508a 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -16,10 +16,13 @@ compiler:
 
 addons:
   apt:
+    sources:
+    - ubuntu-toolchain-r-test
     packages:
     - language-pack-is
     - git-svn
     - apache2
+    - gcc-6
 
 matrix:
   include:
diff --git a/ci/run-build.sh b/ci/run-build.sh
index 4f940d1032..04e163359c 100755
--- a/ci/run-build.sh
+++ b/ci/run-build.sh
@@ -5,4 +5,19 @@
 
 . ${0%/*}/lib-travisci.sh
 
+if [ "$jobname" = linux-gcc ]; then
+	gcc-6 --version
+	cat >config.mak <<-EOF
+	CC=gcc-6
+	CFLAGS = -g -O2 -Wall
+	CFLAGS += -Wextra
+	CFLAGS += -Wmissing-prototypes
+	CFLAGS += -Wno-empty-body
+	CFLAGS += -Wno-maybe-uninitialized
+	CFLAGS += -Wno-missing-field-initializers
+	CFLAGS += -Wno-sign-compare
+	CFLAGS += -Wno-unused-function
+	CFLAGS += -Wno-unused-parameter
+	EOF
+fi
 make --jobs=2
-- 
2.16.1.435.g8f24da2e1a

[PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Nguyễn Thái Ngọc Duy]

We have DEVELOPER config to enable more warnings, but since we can't set
a fixed gcc version to all devs, that has to be a bit more conservative.
On travis, we know almost exact version to be used (bumped to 6.x for
more warnings), we could be more aggressive.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I still think it's a good thing to do. So here's the rebased version
 (v1 collides badly with another series, which has landed)

 .travis.yml               |  3 +++
 ci/run-build-and-tests.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 5f5ee4f3bd..0b3c50f5e7 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -16,10 +16,13 @@ compiler:
 
 addons:
   apt:
+    sources:
+    - ubuntu-toolchain-r-test
     packages:
     - language-pack-is
     - git-svn
     - apache2
+    - gcc-6
 
 matrix:
   include:
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 3735ce413f..f6f346c468 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -7,6 +7,22 @@
 
 ln -s "$cache_dir/.prove" t/.prove
 
+if [ "$jobname" = linux-gcc ]; then
+	gcc-6 --version
+	cat >config.mak <<-EOF
+	CC=gcc-6
+	CFLAGS = -g -O2 -Wall
+	CFLAGS += -Wextra
+	CFLAGS += -Wmissing-prototypes
+	CFLAGS += -Wno-empty-body
+	CFLAGS += -Wno-maybe-uninitialized
+	CFLAGS += -Wno-missing-field-initializers
+	CFLAGS += -Wno-sign-compare
+	CFLAGS += -Wno-unused-function
+	CFLAGS += -Wno-unused-parameter
+	EOF
+fi
+
 make --jobs=2
 make --quiet test
 if test "$jobname" = "linux-gcc"
-- 
2.16.2.903.gd04caf5039

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Jeff King]

On Fri, Mar 16, 2018 at 08:33:55PM +0100, Nguyễn Thái Ngọc Duy wrote:

> We have DEVELOPER config to enable more warnings, but since we can't set
> a fixed gcc version to all devs, that has to be a bit more conservative.
> On travis, we know almost exact version to be used (bumped to 6.x for
> more warnings), we could be more aggressive.

Certainly it makes sense to dial up any static checks we can for the
Travis environment, but...

> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 3735ce413f..f6f346c468 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -7,6 +7,22 @@
>  
>  ln -s "$cache_dir/.prove" t/.prove
>  
> +if [ "$jobname" = linux-gcc ]; then
> +	gcc-6 --version
> +	cat >config.mak <<-EOF
> +	CC=gcc-6
> +	CFLAGS = -g -O2 -Wall
> +	CFLAGS += -Wextra
> +	CFLAGS += -Wmissing-prototypes
> +	CFLAGS += -Wno-empty-body
> +	CFLAGS += -Wno-maybe-uninitialized
> +	CFLAGS += -Wno-missing-field-initializers
> +	CFLAGS += -Wno-sign-compare
> +	CFLAGS += -Wno-unused-function
> +	CFLAGS += -Wno-unused-parameter
> +	EOF
> +fi

Why isn't this just turning on DEVELOPER=1 if we know we have a capable
compiler?

-Peff

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Duy Nguyen]

On Fri, Mar 16, 2018 at 10:22 PM, Jeff King <peff@peff.net> wrote:
>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>> index 3735ce413f..f6f346c468 100755
>> --- a/ci/run-build-and-tests.sh
>> +++ b/ci/run-build-and-tests.sh
>> @@ -7,6 +7,22 @@
>>
>>  ln -s "$cache_dir/.prove" t/.prove
>>
>> +if [ "$jobname" = linux-gcc ]; then
>> +     gcc-6 --version
>> +     cat >config.mak <<-EOF
>> +     CC=gcc-6
>> +     CFLAGS = -g -O2 -Wall
>> +     CFLAGS += -Wextra
>> +     CFLAGS += -Wmissing-prototypes
>> +     CFLAGS += -Wno-empty-body
>> +     CFLAGS += -Wno-maybe-uninitialized
>> +     CFLAGS += -Wno-missing-field-initializers
>> +     CFLAGS += -Wno-sign-compare
>> +     CFLAGS += -Wno-unused-function
>> +     CFLAGS += -Wno-unused-parameter
>> +     EOF
>> +fi
>
> Why isn't this just turning on DEVELOPER=1 if we know we have a capable
> compiler?

DEVELOPER=1 is always set even before this patch. It's set and
exported in lib-travisci.sh.
-- 
Duy

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Lars Schneider]

> On 17 Mar 2018, at 09:01, Duy Nguyen <pclouds@gmail.com> wrote:
> 
> On Fri, Mar 16, 2018 at 10:22 PM, Jeff King <peff@peff.net> wrote:
>>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>>> index 3735ce413f..f6f346c468 100755
>>> --- a/ci/run-build-and-tests.sh
>>> +++ b/ci/run-build-and-tests.sh
>>> @@ -7,6 +7,22 @@
>>> 
>>> ln -s "$cache_dir/.prove" t/.prove
>>> 
>>> +if [ "$jobname" = linux-gcc ]; then
>>> +     gcc-6 --version
>>> +     cat >config.mak <<-EOF
>>> +     CC=gcc-6
>>> +     CFLAGS = -g -O2 -Wall
>>> +     CFLAGS += -Wextra
>>> +     CFLAGS += -Wmissing-prototypes
>>> +     CFLAGS += -Wno-empty-body
>>> +     CFLAGS += -Wno-maybe-uninitialized
>>> +     CFLAGS += -Wno-missing-field-initializers
>>> +     CFLAGS += -Wno-sign-compare
>>> +     CFLAGS += -Wno-unused-function
>>> +     CFLAGS += -Wno-unused-parameter
>>> +     EOF
>>> +fi
>> 
>> Why isn't this just turning on DEVELOPER=1 if we know we have a capable
>> compiler?
> 
> DEVELOPER=1 is always set even before this patch. It's set and
> exported in lib-travisci.sh.

I interpreted Peff's comment like this:

If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, 
then we could set your additional flags in the makefile.

This way every developer with a new compiler would run these
flags locally (if DEVELOPER=1 is set).

- Lars

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Duy Nguyen]

On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:
> I interpreted Peff's comment like this:
> 
> If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, 
> then we could set your additional flags in the makefile.
> 
> This way every developer with a new compiler would run these
> flags locally (if DEVELOPER=1 is set).

Aha. Something like this? I split developer cflags out to a separate
file because I imagine people may start to add gcc7, clang....

-- 8< --
diff --git a/.travis.yml b/.travis.yml
index 5f5ee4f3bd..0b3c50f5e7 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -16,10 +16,13 @@ compiler:
 
 addons:
   apt:
+    sources:
+    - ubuntu-toolchain-r-test
     packages:
     - language-pack-is
     - git-svn
     - apache2
+    - gcc-6
 
 matrix:
   include:
diff --git a/Makefile b/Makefile
index de4b8f0c02..e8321e44d6 100644
--- a/Makefile
+++ b/Makefile
@@ -436,15 +436,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)
@@ -1045,7 +1036,8 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
+CFLAGS += $(DEV_CFLAGS)
 endif
 
 comma := ,
diff --git a/config.mak.dev b/config.mak.dev
index e69de29bb2..644d0d581f 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -0,0 +1,23 @@
+DEV_CFLAGS =
+DEV_CFLAGS += -Werror
+DEV_CFLAGS += -Wdeclaration-after-statement
+DEV_CFLAGS += -Wno-format-zero-length
+DEV_CFLAGS += -Wold-style-definition
+DEV_CFLAGS += -Woverflow
+DEV_CFLAGS += -Wpointer-arith
+DEV_CFLAGS += -Wstrict-prototypes
+DEV_CFLAGS += -Wunused
+DEV_CFLAGS += -Wvla
+
+GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) -dumpversion | cut -f 1 -d .')
+
+ifeq ($(GCC_VER),6)
+DEV_CFLAGS += -Wextra
+DEV_CFLAGS += -Wmissing-prototypes
+DEV_CFLAGS += -Wno-empty-body
+DEV_CFLAGS += -Wno-maybe-uninitialized
+DEV_CFLAGS += -Wno-missing-field-initializers
+DEV_CFLAGS += -Wno-sign-compare
+DEV_CFLAGS += -Wno-unused-function
+DEV_CFLAGS += -Wno-unused-parameter
+endif
-- 8< --

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Jeff King]

On Sat, Mar 17, 2018 at 03:59:23PM +0100, Duy Nguyen wrote:

> On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:
> > I interpreted Peff's comment like this:
> > 
> > If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, 
> > then we could set your additional flags in the makefile.
> > 
> > This way every developer with a new compiler would run these
> > flags locally (if DEVELOPER=1 is set).
> 
> Aha. Something like this? I split developer cflags out to a separate
> file because I imagine people may start to add gcc7, clang....

Yeah, gcc 7 is already standard on debian unstable, so...

> +GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) -dumpversion | cut -f 1 -d .')
> +
> +ifeq ($(GCC_VER),6)

...this probably needs to be "greater than or equal to".

Unfortunately I think that's hard to do in pure make. But we're already
relying on $(shell) here, so we could just move the logic there.

Something like the patch below, perhaps. It should do the right thing on
clang and gcc, and I added in an extra clang-only warning I've found
useful. Otherwise the list of flags comes from your patch.

The "clang4" limit is just what I happened to have on my system to test
with. Some of those flags might work for clang3, and some of the
gcc6-only flags might work on newer versions of clang.

I was puzzled by -Wno-maybe-uninitialized, though. I think that has
found bugs for us before (but also false positives, but we usually
squelch those in the code). And it's part of -Wall, not -Wextra, so we
shouldn't need it as part of this patch, should we?

diff --git a/Makefile b/Makefile
index a1d8775adb..9dfd152a1e 100644
--- a/Makefile
+++ b/Makefile
@@ -442,15 +442,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 +1042,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 0000000000..59aef342c4
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,28 @@
+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
+
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+
+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
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 0000000000..043367828c
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+print_flags() {
+	family=$1
+	version=$($CC -dumpversion | cut -f 1 -d .)
+
+	# Print a feature flag not only for the current version, but also
+	# for any prior versions we encompass. This avoids needing to do
+	# numeric comparisons in make, which are awkward.
+	while test "$version" -gt 0
+	do
+		echo $family$version
+		version=$((version - 1))
+	done
+}
+
+case "$($CC --version 2>&1)" in
+gcc*)
+	print_flags gcc
+	;;
+clang*)
+	print_flags clang
+	;;
+*)
+	: unknown compiler family
+	;;
+esac
-- 
2.17.0.rc0.402.ged0b3fd1ee

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Jeff King]

On Sat, Mar 17, 2018 at 12:08:32PM -0400, Jeff King wrote:

> +case "$($CC --version 2>&1)" in
> +gcc*)
> +	print_flags gcc
> +	;;
> +clang*)
> +	print_flags clang
> +	;;
> +*)
> +	: unknown compiler family
> +	;;
> +esac

By the way, one annoying thing is that running "cc --version" when "cc"
is actually "gcc" will print something like:

  cc (Debian 7.3.0-11) 7.3.0

even though it supports all of the gcc knobs.  This means that:

  make DEVELOPER=1

without further config won't get these knobs, because we (rightly)
default CC=cc.

Probably this detection could be a bit more clever, like:

  cc*Free Software Foundation")
	print_flags gcc

or something. I don't have any non-gcc/clang compilers to test with, so
I'm not sure what they even print for "--version" (if anything).

-Peff

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Unfortunately I think that's hard to do in pure make. But we're already
> relying on $(shell) here, so we could just move the logic there.
>
> Something like the patch below, perhaps. It should do the right thing on
> clang and gcc, and I added in an extra clang-only warning I've found
> useful. Otherwise the list of flags comes from your patch.

I see this discussion is going in the right direction, and like the
approach taken by this "how about this" patch.

Thanks.

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

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

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 -dumpversion does not work correctly for clang. So I use "-v" instead
 which seems to produce the same output for both gcc and clang (with a
 minor difference in freebsd where it says "FreeBSD clang" instead of
 just "clang"). Not sure if it helps your "cc on debian" case though

 Tested with clang-5.0.1 and gcc-6.4.0 (too lazy to test on freebsd
 clang 3.4.1 but I don't expect surprises there)

 I will still need to pull more modern gcc/clang on travis, but that
 can wait until this patch settles.

 Makefile        | 11 +----------
 config.mak.dev  | 28 +++++++++++++++++++++++++++
 detect-compiler | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

diff --git a/Makefile b/Makefile
index a1d8775adb..9dfd152a1e 100644
--- a/Makefile
+++ b/Makefile
@@ -442,15 +442,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 +1042,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 0000000000..59aef342c4
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,28 @@
+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
+
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+
+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
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 0000000000..bc2ea39ef5
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,50 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+# we get something like (this is at least true for gcc and clang)
+#
+# FreeBSD clang version 3.4.1 (tags/RELEASE...)
+get_version_line() {
+	"$CC" -v 2>&1 | grep ' version '
+}
+
+get_family() {
+	get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
+}
+
+get_version() {
+	get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'
+}
+
+print_flags() {
+	family=$1
+	version=$(get_version | cut -f 1 -d .)
+
+	# Print a feature flag not only for the current version, but also
+	# for any prior versions we encompass. This avoids needing to do
+	# numeric comparisons in make, which are awkward.
+	while test "$version" -gt 0
+	do
+		echo $family$version
+		version=$((version - 1))
+	done
+}
+
+case "$(get_family)" in
+gcc)
+	print_flags gcc
+	;;
+*clang)
+	print_flags clang
+	;;
+"FreeBSD clang")
+	print_flags clang
+	;;
+*)
+	: unknown compiler family
+	;;
+esac
-- 
2.17.0.rc0.347.gf9cf61673a

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

[Eric Sunshine]

On Sun, Mar 18, 2018 at 09:18:34AM +0100, 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.
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/detect-compiler b/detect-compiler
> --- /dev/null
> +++ b/detect-compiler
> @@ -0,0 +1,50 @@
> +get_version_line() {
> +	"$CC" -v 2>&1 | grep ' version '
> +}

On MacOS, "cc -v" output is:

--- >8 ---
Apple LLVM version 9.0.0 (clang-900.0.39.2)
Target: x86_64-apple-darwin16.7.0
Thread model: posix
InstalledDir: ...
--- >8 ---

> +get_family() {
> +	get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
> +}

So, this returns "Apple LLVM".

> +case "$(get_family)" in
> +gcc)
> +	print_flags gcc
> +	;;
> +*clang)
> +	print_flags clang
> +	;;
> +"FreeBSD clang")
> +	print_flags clang
> +	;;
> +*)
> +	: unknown compiler family
> +	;;
> +esac

Which means you probably want to squash in:

--- >8 ---
diff --git a/detect-compiler b/detect-compiler
index bc2ea39ef5..3140416b40 100755
--- a/detect-compiler
+++ b/detect-compiler
@@ -41,6 +41,9 @@ gcc)
 *clang)
 	print_flags clang
 	;;
+"Apple LLVM")
+	print_flags clang
+	;;
 "FreeBSD clang")
 	print_flags clang
 	;;
--- >8 ---

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

[Duy Nguyen]

On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Mar 18, 2018 at 09:18:34AM +0100, 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.
>>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> diff --git a/detect-compiler b/detect-compiler
>> --- /dev/null
>> +++ b/detect-compiler
>> @@ -0,0 +1,50 @@
>> +get_version_line() {
>> +     "$CC" -v 2>&1 | grep ' version '
>> +}
>
> On MacOS, "cc -v" output is:
>
> --- >8 ---
> Apple LLVM version 9.0.0 (clang-900.0.39.2)
> Target: x86_64-apple-darwin16.7.0
> Thread model: posix
> InstalledDir: ...
> --- >8 ---

Does it still build ok for you with your changes squashed in? I think
the check for clang4/gcc6 and setting -Wextra in config.mak.dev may
backfire because clang9 probably has a lot more warnings enabled and
some of them we don't want and cause compile error...
-- 
Duy

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

[Jeff King]

On Sun, Mar 18, 2018 at 10:17:41AM +0100, Duy Nguyen wrote:

> On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Sun, Mar 18, 2018 at 09:18:34AM +0100, 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.
> >>
> >> Helped-by: Jeff King <peff@peff.net>
> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >> ---
> >> diff --git a/detect-compiler b/detect-compiler
> >> --- /dev/null
> >> +++ b/detect-compiler
> >> @@ -0,0 +1,50 @@
> >> +get_version_line() {
> >> +     "$CC" -v 2>&1 | grep ' version '
> >> +}
> >
> > On MacOS, "cc -v" output is:
> >
> > --- >8 ---
> > Apple LLVM version 9.0.0 (clang-900.0.39.2)
> > Target: x86_64-apple-darwin16.7.0
> > Thread model: posix
> > InstalledDir: ...
> > --- >8 ---
> 
> Does it still build ok for you with your changes squashed in? I think
> the check for clang4/gcc6 and setting -Wextra in config.mak.dev may
> backfire because clang9 probably has a lot more warnings enabled and
> some of them we don't want and cause compile error...

I don't think we can predict that going forward, though. We'd have to
wait for people to use the new compiler and then send a patch disabling
whatever causes a problem (or better yet, fixing the code if
appropriate).

This framework can only help us write those patches; I don't think it
can save us from having to do them in the first place.

-Peff

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

[Eric Sunshine]

On Sun, Mar 18, 2018 at 5:17 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On MacOS, "cc -v" output is:
>> --- >8 ---
>> Apple LLVM version 9.0.0 (clang-900.0.39.2)
>> Target: x86_64-apple-darwin16.7.0
>> Thread model: posix
>> InstalledDir: ...
>> --- >8 ---
>
> Does it still build ok for you with your changes squashed in? I think
> the check for clang4/gcc6 and setting -Wextra in config.mak.dev may
> backfire because clang9 probably has a lot more warnings enabled and
> some of them we don't want and cause compile error...

The project does still compile cleanly for me. (And I used "make V=1"
to verify that the new flags are used.)

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

[Jeff King]

On Sun, Mar 18, 2018 at 05:06:07AM -0400, Eric Sunshine wrote:

> On MacOS, "cc -v" output is:
> 
> --- >8 ---
> Apple LLVM version 9.0.0 (clang-900.0.39.2)
> Target: x86_64-apple-darwin16.7.0
> Thread model: posix
> InstalledDir: ...
> --- >8 ---

Is that really way ahead of the clang releases (which are at 7.0,
AFAIK), or do they use a totally different number scheme?

If the latter, I guess we'd have to treat it separately from clang and
have all of the conditionals treat it independently? Yuck.

-Peff

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

[Eric Sunshine]

On Sun, Mar 18, 2018 at 5:28 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Mar 18, 2018 at 05:06:07AM -0400, Eric Sunshine wrote:
>> On MacOS, "cc -v" output is:
>> --- >8 ---
>> Apple LLVM version 9.0.0 (clang-900.0.39.2)
>> Target: x86_64-apple-darwin16.7.0
>> Thread model: posix
>> InstalledDir: ...
>> --- >8 ---
>
> Is that really way ahead of the clang releases (which are at 7.0,
> AFAIK), or do they use a totally different number scheme?

I have no idea, but from past experience with Apple, I'd guess that
the version number is an Apple invention.

{...goes and researches a bit...}

Indeed, there doesn't seem to be any correlation between what Apple
reports and the actual clang version number[1].

[1]: https://stackoverflow.com/questions/33603027/get-apple-clang-version-and-corresponding-upstream-llvm-version

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

[Jeff King]

On Sun, Mar 18, 2018 at 09:18:34AM +0100, 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.

Hrm, I was planning to expand my patch into a series. In particular, in
my experiments some of those warnings do not need to be disabled under
-Wextra (I tested going back to gcc 5 and clang 4.0).

I think it's worth splitting up the boilerplate changes from the
decisions on individual warnings, because I suspect we'll need to revise
the latter going forward.

I also noticed that all of those options seem to go back at least to
gcc-5. It makes me wonder how much we even need this version-detecting
framework. There are one or two "clang vs gcc" things I've found, but I
haven't found a case where old versions of gcc don't support a
particular option.

IOW, could we get away with just adding these to the DEVELOPER knob and
assuming that people who use it have a reasonably modern gcc or clang
setup? That's more or less what the existing knobs do.

But I didn't follow whether there was any earlier discussion on specific
problems.

>  -dumpversion does not work correctly for clang. So I use "-v" instead
>  which seems to produce the same output for both gcc and clang (with a
>  minor difference in freebsd where it says "FreeBSD clang" instead of
>  just "clang"). Not sure if it helps your "cc on debian" case though

Heh. At first I was confused, as it seems to work for me:

  $ clang-4.0 -dumpversion
  4.2.1

But then I tried this:

  $ clang-5.0 -dumpversion
  4.2.1

Whoops. It returns the same value up through clang 7.

-Peff

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

[Duy Nguyen]

On Sun, Mar 18, 2018 at 10:26 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Mar 18, 2018 at 09:18:34AM +0100, 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.
>
> Hrm, I was planning to expand my patch into a series.

I started this so I think people expect me to move it forward. But
I'll gladly hand it to you. It looks like you have more compilers to
play with anyway (I'm on gentoo and installing a new compiler,
especially llvm-based, takes ages).
-- 
Duy

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

[Duy Nguyen]

On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
> +CFLAGS += -Wextra

Another thing we can add here is -Og instead of standard -O2 (or -O0
in my build), which is supported since gcc 4.8. clang seems to support
it too (mapped to -O1 at least for clang5) but I don't know what
version added that flag.
-- 
Duy

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

[Ramsay Jones]

On 18/03/18 15:55, Duy Nguyen wrote:
> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
>> +CFLAGS += -Wextra
> 
> Another thing we can add here is -Og instead of standard -O2 (or -O0
> in my build), which is supported since gcc 4.8. clang seems to support
> it too (mapped to -O1 at least for clang5) but I don't know what
> version added that flag.

I've been doing a lot of compiling recently, using 10 'different'
versions of clang and gcc ('different' if you count 64-bit and 32-bit
compilers with the same version number as different!)

However, I can tell you that clang version 3.4 and 3.8.0 don't
support -Og, but clang version 5.0.1 does.

ATB,
Ramsay Jones

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

[Duy Nguyen]

On Sun, Mar 18, 2018 at 7:56 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 18/03/18 15:55, Duy Nguyen wrote:
>> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
>>> +CFLAGS += -Wextra
>>
>> Another thing we can add here is -Og instead of standard -O2 (or -O0
>> in my build), which is supported since gcc 4.8. clang seems to support
>> it too (mapped to -O1 at least for clang5) but I don't know what
>> version added that flag.
>
> I've been doing a lot of compiling recently, using 10 'different'
> versions of clang and gcc ('different' if you count 64-bit and 32-bit
> compilers with the same version number as different!)
>
> However, I can tell you that clang version 3.4 and 3.8.0 don't
> support -Og, but clang version 5.0.1 does.

Yeah. I checked clang git mirror and this -Og is in 4.x release branch
(couldn't nail down exact release) so 5.x should be a safe bet.
-- 
Duy

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

[Jeff King]

On Sun, Mar 18, 2018 at 04:55:25PM +0100, Duy Nguyen wrote:

> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
> > +CFLAGS += -Wextra
> 
> Another thing we can add here is -Og instead of standard -O2 (or -O0
> in my build), which is supported since gcc 4.8. clang seems to support
> it too (mapped to -O1 at least for clang5) but I don't know what
> version added that flag.

I don't know, that feels kind of weird to me. I thought DEVELOPER was
supposed to mean "ratchet up the linting, I want to know if I've broken
something".

But tweaking -O is about "I am in an edit-compile-debug cycle".
Sometimes you are and sometimes you aren't, but you'd presumably want to
have extra warnings regardless (especially because some warnings only
trigger under particular optimization settings).

Personally, I default to -O0, but then crank up to -O2 for performance
testing or for my daily-driver builds. But I _always_ have as many
warnings enabled as possible[1].

-Peff

[1] I do have some pretty horrific magic to turn on -Werror when I'm
    visiting a "historical" commit, such as during a bisect.

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

[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 <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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 0000000000..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
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 0000000000..70b754481c
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+# we get something like (this is at least true for gcc and clang)
+#
+# FreeBSD clang version 3.4.1 (tags/RELEASE...)
+get_version_line() {
+	$CC -v 2>&1 | grep ' version '
+}
+
+get_family() {
+	get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
+}
+
+get_version() {
+	get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'
+}
+
+print_flags() {
+	family=$1
+	version=$(get_version | cut -f 1 -d .)
+
+	# Print a feature flag not only for the current version, but also
+	# for any prior versions we encompass. This avoids needing to do
+	# numeric comparisons in make, which are awkward.
+	while test "$version" -gt 0
+	do
+		echo $family$version
+		version=$((version - 1))
+	done
+}
+
+case "$(get_family)" in
+gcc)
+	print_flags gcc
+	;;
+clang)
+	print_flags clang
+	;;
+"FreeBSD clang")
+	print_flags clang
+	;;
+"Apple LLVM")
+	print_flags clang
+	;;
+*)
+	: unknown compiler family
+	;;
+esac
-- 
2.17.0.rc0.348.gd5a49e0b6f

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

[Eric Sunshine]

On Sat, Mar 24, 2018 at 8:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <peff@peff.net>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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

[Duy Nguyen]

On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> 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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

 On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano <gitster@pobox.com> 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

[PATCH v3 0/3] Enable more compiler warnings for devs

[Nguyễn Thái Ngọc Duy]

v3 fixes the fallthru warnings in connect.c, and with json-writer
series rerolled, 'pu' should build cleanly now.
-Wno-maybe-uninitialized is removed, thanks to Ramsay.

v3 also adds an experimental patch that adds EAGER_DEVELOPER=1. These
developers will have all warnings enabled (nothing is suppressed) but
they are not fatal. They could go through them and perhaps clean up
the code base a bit more.

Interdiff

diff --git a/Makefile b/Makefile
index e6680a8977..e4f04ce1cb 100644
--- a/Makefile
+++ b/Makefile
@@ -434,7 +434,9 @@ all::
 #
 # 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)
+# COMPILER_FEATURES (see config.mak.dev).
+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
+# class is suppressed anymore.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1041,6 +1043,9 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
+ifdef EAGER_DEVELOPER
+DEVELOPER = Yes
+endif
 ifdef DEVELOPER
 include config.mak.dev
 endif
diff --git a/config.mak.dev b/config.mak.dev
index d8beaf9347..13883410b3 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,4 +1,6 @@
+ifndef EAGER_DEVELOPER
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
@@ -18,13 +20,23 @@ endif
 
 ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
 CFLAGS += -Wextra
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifndef EAGER_DEVELOPER
+# These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
 CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
-ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
-CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
 endif
 endif
diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));


Nguyễn Thái Ngọc Duy (3):
  connect.c: mark die_initial_contact() NORETURN
  Makefile: detect compiler and enable more warnings in DEVELOPER=1
  Makefile: add EAGER_DEVELOPER mode

 Makefile        | 20 +++++++++----------
 config.mak.dev  | 42 +++++++++++++++++++++++++++++++++++++++
 connect.c       |  2 +-
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 11 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

-- 
2.17.0.rc1.439.gca064e2955

[PATCH v3 1/3] connect.c: mark die_initial_contact() NORETURN

[Nguyễn Thái Ngọc Duy]

There is a series running in parallel with this one that adds code
like this

    switch (...) {
    case ...:
        die_initial_contact();
    case ...:

There is nothing wrong with this. There is no actual falling
through. But since gcc is not that smart and gcc 7.x introduces
-Wimplicit-fallthrough, it raises a false alarm in this case.

This class of warnings may be useful elsewhere, so instead of
suppressing the whole class, let's try to fix just this code. gcc is
smart enough to realize that no execution can continue after a
NORETURN function call and no longer raises the warning.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));
-- 
2.17.0.rc1.439.gca064e2955

[PATCH v3 2/3] Makefile: detect compiler and enable more warnings in DEVELOPER=1

[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 those warnings are
probably worth fixing instead of just suppressing in future.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile        | 15 +++++---------
 config.mak.dev  | 38 +++++++++++++++++++++++++++++++++++
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

diff --git a/Makefile b/Makefile
index 7f40f76739..e6680a8977 100644
--- a/Makefile
+++ b/Makefile
@@ -431,6 +431,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
@@ -439,15 +443,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)
@@ -1047,7 +1042,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 0000000000..716a14ecc7
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,38 @@
+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
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
+CFLAGS += -Wmissing-prototypes
+# These are disabled because we have these all over the place.
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 0000000000..70b754481c
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+# we get something like (this is at least true for gcc and clang)
+#
+# FreeBSD clang version 3.4.1 (tags/RELEASE...)
+get_version_line() {
+	$CC -v 2>&1 | grep ' version '
+}
+
+get_family() {
+	get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
+}
+
+get_version() {
+	get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'
+}
+
+print_flags() {
+	family=$1
+	version=$(get_version | cut -f 1 -d .)
+
+	# Print a feature flag not only for the current version, but also
+	# for any prior versions we encompass. This avoids needing to do
+	# numeric comparisons in make, which are awkward.
+	while test "$version" -gt 0
+	do
+		echo $family$version
+		version=$((version - 1))
+	done
+}
+
+case "$(get_family)" in
+gcc)
+	print_flags gcc
+	;;
+clang)
+	print_flags clang
+	;;
+"FreeBSD clang")
+	print_flags clang
+	;;
+"Apple LLVM")
+	print_flags clang
+	;;
+*)
+	: unknown compiler family
+	;;
+esac
-- 
2.17.0.rc1.439.gca064e2955

[PATCH v3 3/3] Makefile: add EAGER_DEVELOPER mode

[Nguyễn Thái Ngọc Duy]

This mode is for developers who want to keep the code base
clean. There are warning classes that are currently suppressed because
we have too many of them in the code base. This mode will stop the
suppression, let the developer see and decide how to fix them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile       | 7 ++++++-
 config.mak.dev | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e6680a8977..e4f04ce1cb 100644
--- a/Makefile
+++ b/Makefile
@@ -434,7 +434,9 @@ all::
 #
 # 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)
+# COMPILER_FEATURES (see config.mak.dev).
+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
+# class is suppressed anymore.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1041,6 +1043,9 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
+ifdef EAGER_DEVELOPER
+DEVELOPER = Yes
+endif
 ifdef DEVELOPER
 include config.mak.dev
 endif
diff --git a/config.mak.dev b/config.mak.dev
index 716a14ecc7..13883410b3 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,4 +1,6 @@
+ifndef EAGER_DEVELOPER
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
@@ -21,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifndef EAGER_DEVELOPER
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining
-- 
2.17.0.rc1.439.gca064e2955

[PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Ævar Arnfjörð Bjarmason]

Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag
which (approximately) enables -Wextra so that any combination of them
and -Werror can be set.

I've long wanted to use DEVELOPER=1 in my production builds, but on
some old systems I still get warnings, and thus the build would
fail. However if the build/tests fail for some other reason, it would
still be useful to scroll up and see what the relevant code is warning
about.

This change allows for that. Now setting DEVELOPER will set -Werror as
before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,
but without -Werror.

I've renamed the newly added EAGER_DEVELOPER flag to
DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,
and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than
inventing some new name of our own (VERY_EAGER_DEVELOPER?).

The DEVELOPER_EXTRA flag implicitly means DEVELOPER_NONFATAL, but just
so that this change doesn't introduce yet another arbitrary limitation
it's possible to combine it with DEVELOPER_FATAL, which will turn on
-Werror for it as well.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I really like where this is going, but as noted I think this on top
would be even better.

 Makefile       | 17 +++++++++++------
 config.mak.dev | 10 ++++++++--
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index e4f04ce1cb..641461a569 100644
--- a/Makefile
+++ b/Makefile
@@ -432,11 +432,16 @@ 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).
-# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning
-# class is suppressed anymore.
+# Define DEVELOPER to enable more compiler warnings. We'll also enable
+# -Werror unless DEVELOPER_NONFATAL is defined. To enable even more
+# pedantic warnings that'll flag some potential existing issues in the
+# codebase turn on DEVELOPER_EXTRA, which implicitly sets DEVELOPER as
+# well, This is -Wextra with a whitelist of disabled warnings. Unless
+# DEVELOPER_NONFATAL is set DEVELOPER_EXTRA will turn it on
+# implicitly, so if you for some reason want both DEVELOPER and
+# DEVELOPER_EXTRA with fatal warnings, you need to set
+# DEVELOPER_FATAL=1 to force -Werror. See config.mak.dev for how this
+# all works.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1043,7 +1048,7 @@ include config.mak.uname
 -include config.mak.autogen
 -include config.mak
 
-ifdef EAGER_DEVELOPER
+ifdef DEVELOPER_EXTRA
 DEVELOPER = Yes
 endif
 ifdef DEVELOPER
diff --git a/config.mak.dev b/config.mak.dev
index 13883410b3..40f3365729 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,6 +1,12 @@
-ifndef EAGER_DEVELOPER
+ifndef DEVELOPER_NONFATAL
+ifndef DEVELOPER_EXTRA
 CFLAGS += -Werror
 endif
+endif
+ifdef DEVELOPER_FATAL
+CFLAGS += -Werror
+endif
+
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
@@ -23,7 +29,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
-ifndef EAGER_DEVELOPER
+ifndef DEVELOPER_EXTRA
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
-- 
2.17.0.rc1.321.gba9d0f2565

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Duy Nguyen]

On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag
> which (approximately) enables -Wextra so that any combination of them
> and -Werror can be set.
>
> I've long wanted to use DEVELOPER=1 in my production builds, but on
> some old systems I still get warnings, and thus the build would
> fail. However if the build/tests fail for some other reason, it would
> still be useful to scroll up and see what the relevant code is warning
> about.
>
> This change allows for that. Now setting DEVELOPER will set -Werror as
> before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,
> but without -Werror.
>
> I've renamed the newly added EAGER_DEVELOPER flag to
> DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,
> and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than
> inventing some new name of our own (VERY_EAGER_DEVELOPER?).

Before we go with zillions of *DEVELOPER* maybe we can have something
like DEVOPTS where you can give multiple keywords to a single variable
to influence config.mak.dev. This is similar to COMPILER_FEATURES we
already have in there, but now it's driven by the dev instead of the
compiler. So you can have keywords like "gentle" (no -Werror) "extra"
(-Wextra with no suppression) and something else.
-- 
Duy

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Ævar Arnfjörð Bjarmason]

On Sat, Mar 31 2018, Duy Nguyen wrote:

> On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag
>> which (approximately) enables -Wextra so that any combination of them
>> and -Werror can be set.
>>
>> I've long wanted to use DEVELOPER=1 in my production builds, but on
>> some old systems I still get warnings, and thus the build would
>> fail. However if the build/tests fail for some other reason, it would
>> still be useful to scroll up and see what the relevant code is warning
>> about.
>>
>> This change allows for that. Now setting DEVELOPER will set -Werror as
>> before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,
>> but without -Werror.
>>
>> I've renamed the newly added EAGER_DEVELOPER flag to
>> DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,
>> and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than
>> inventing some new name of our own (VERY_EAGER_DEVELOPER?).
>
> Before we go with zillions of *DEVELOPER* maybe we can have something
> like DEVOPTS where you can give multiple keywords to a single variable
> to influence config.mak.dev. This is similar to COMPILER_FEATURES we
> already have in there, but now it's driven by the dev instead of the
> compiler. So you can have keywords like "gentle" (no -Werror) "extra"
> (-Wextra with no suppression) and something else.

We could do that, but I don't think it's that bad. This patch is one
extra option on top of yours, and it's not going to result in some
combinatorial explosion of options, i.e. if we add DEVELOPER_PEDANTIC
we'll just add one extra flag.

But sure, we could make this some string we'd need to parse out similar
to COMPILER_FEATURES, it just seems more complex to me for this task.

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Duy Nguyen]

On Tue, Apr 03, 2018 at 11:19:46AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sat, Mar 31 2018, Duy Nguyen wrote:
> 
> > On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >> Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag
> >> which (approximately) enables -Wextra so that any combination of them
> >> and -Werror can be set.
> >>
> >> I've long wanted to use DEVELOPER=1 in my production builds, but on
> >> some old systems I still get warnings, and thus the build would
> >> fail. However if the build/tests fail for some other reason, it would
> >> still be useful to scroll up and see what the relevant code is warning
> >> about.
> >>
> >> This change allows for that. Now setting DEVELOPER will set -Werror as
> >> before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,
> >> but without -Werror.
> >>
> >> I've renamed the newly added EAGER_DEVELOPER flag to
> >> DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,
> >> and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than
> >> inventing some new name of our own (VERY_EAGER_DEVELOPER?).
> >
> > Before we go with zillions of *DEVELOPER* maybe we can have something
> > like DEVOPTS where you can give multiple keywords to a single variable
> > to influence config.mak.dev. This is similar to COMPILER_FEATURES we
> > already have in there, but now it's driven by the dev instead of the
> > compiler. So you can have keywords like "gentle" (no -Werror) "extra"
> > (-Wextra with no suppression) and something else.
> 
> We could do that, but I don't think it's that bad. This patch is one
> extra option on top of yours,

And that eager this was marked experimental because I was not sure if
it was the right way :)

> and it's not going to result in some combinatorial explosion of
> options, i.e. if we add DEVELOPER_PEDANTIC we'll just add one extra
> flag.
> 
> But sure, we could make this some string we'd need to parse out similar
> to COMPILER_FEATURES, it just seems more complex to me for this task.

It's not that complex. With the EAGER_DEVELOPER patch removed, we can
have something like this where eager devs just need to put

    DEVOPTS = gentle no-suppression

and you put

    DEVOPTS = gentle

(bad naming, I didn't spend time thinking about names)

-- 8< --
diff --git a/Makefile b/Makefile
index e6680a8977..7b4e038e1d 100644
--- a/Makefile
+++ b/Makefile
@@ -435,6 +435,11 @@ all::
 # 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)
+#
+# When DEVELOPER is set, DEVOPTS can be used to control compiler options.
+# This variable contains keywords separated by whitespace. Two keywords
+# are recognized: "gentle" removes -Werror and "no-suppression"
+# removes all "-Wno-" options.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
diff --git a/config.mak.dev b/config.mak.dev
index 716a14ecc7..1d7aba6a6a 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,4 +1,6 @@
+ifeq ($(filter gentle,$(DEVOPTS)),)
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
@@ -21,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifeq ($(filter no-suppression,$(DEVOPTS)),)
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining
-- 8< --

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Jeff King]

On Tue, Apr 03, 2018 at 05:17:00PM +0200, Duy Nguyen wrote:

> It's not that complex. With the EAGER_DEVELOPER patch removed, we can
> have something like this where eager devs just need to put
> 
>     DEVOPTS = gentle no-suppression
> 
> and you put
> 
>     DEVOPTS = gentle
> 
> (bad naming, I didn't spend time thinking about names)

It seems to me like we're losing the point of DEVELOPER here. I thought
the idea was to have a turn-key flag you could set to get extra linting
on your commits. But now we're tweaking all kinds of individual options.
At some point are we better off just letting you put "-Wno-foo" in your
CFLAGS yourself?

I don't mind the version-based checks because they're automatic, so the
feature remains turn-key. But this kind of DEVOPTS seems like a step in
the wrong direction.

-Peff

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Duy Nguyen]

On Fri, Apr 6, 2018 at 11:42 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 03, 2018 at 05:17:00PM +0200, Duy Nguyen wrote:
>
>> It's not that complex. With the EAGER_DEVELOPER patch removed, we can
>> have something like this where eager devs just need to put
>>
>>     DEVOPTS = gentle no-suppression
>>
>> and you put
>>
>>     DEVOPTS = gentle
>>
>> (bad naming, I didn't spend time thinking about names)
>
> It seems to me like we're losing the point of DEVELOPER here. I thought
> the idea was to have a turn-key flag you could set to get extra linting
> on your commits. But now we're tweaking all kinds of individual options.
> At some point are we better off just letting you put "-Wno-foo" in your
> CFLAGS yourself?

It's because what AEvar wanted is no longer a dev thing :)

> I don't mind the version-based checks because they're automatic, so the
> feature remains turn-key. But this kind of DEVOPTS seems like a step in
> the wrong direction.
>
> -Peff



-- 
Duy

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 07 2018, Duy Nguyen wrote:

> On Fri, Apr 6, 2018 at 11:42 PM, Jeff King <peff@peff.net> wrote:
>> On Tue, Apr 03, 2018 at 05:17:00PM +0200, Duy Nguyen wrote:
>>
>>> It's not that complex. With the EAGER_DEVELOPER patch removed, we can
>>> have something like this where eager devs just need to put
>>>
>>>     DEVOPTS = gentle no-suppression
>>>
>>> and you put
>>>
>>>     DEVOPTS = gentle
>>>
>>> (bad naming, I didn't spend time thinking about names)
>>
>> It seems to me like we're losing the point of DEVELOPER here. I thought
>> the idea was to have a turn-key flag you could set to get extra linting
>> on your commits. But now we're tweaking all kinds of individual options.
>> At some point are we better off just letting you put "-Wno-foo" in your
>> CFLAGS yourself?
>
> It's because what AEvar wanted is no longer a dev thing :)

It's still a dev thing, but just for a slightly different use-case. I
hack my patches up with DEVELOPER=1, and then eventually deploy a
patched git version with those (and others) on top of the latest release
tag.

At that point I want the same compiler assertions, but might be building
on much older compilers, so I just want the warning output without
-Werror.

Anyway, I see you've pushed a new version with DEVOPTS. I'll submit mine
on top of that once your new version lands (unless you want to try to
integrate it yourself).

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Duy Nguyen]

On Sat, Apr 7, 2018 at 2:36 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Anyway, I see you've pushed a new version with DEVOPTS. I'll submit mine
> on top of that once your new version lands (unless you want to try to
> integrate it yourself).

Actually I think I'll just drop both EAGER_DEVELOPER and DEVOTPS.
-- 
Duy

Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 07 2018, Duy Nguyen wrote:

> On Sat, Apr 7, 2018 at 2:36 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Anyway, I see you've pushed a new version with DEVOPTS. I'll submit mine
>> on top of that once your new version lands (unless you want to try to
>> integrate it yourself).
>
> Actually I think I'll just drop both EAGER_DEVELOPER and DEVOTPS.

OK. I'll submit something on top to optionally get rid of -Werror, since
that's the only bit I actually care about & have a use-case for.

[PATCH v4 0/4] Make DEVELOPER more more flexible with DEVOPTS

[Ævar Arnfjörð Bjarmason]

This is a v4 and replacement of gitster/nd/warn-more-for-devs. I'm
sending this with Duy's blessing.

The first two patches are the same, except for one trivial
s/faimily/family/ typo fix.

The third patch in gitster/nd/warn-more-for-devs ("Makefile: add
EAGER_DEVELOPER mode") is gone, instead there's now a DEVOPTS
option. The 3/4 and 4/4 add a way to turn off -Werror & the -Wextra
suppression, respectively.

Duy was right in [1] that this is a much better and extensible way of
doing this than my "Makefile: untangle DEVELOPER and -Werror" patch.

Most of 3/4 & 4/4 are just tweaked from git@github.com:pclouds/git.git
pclouds/more-warnings and combined with my previous 4/3 patch[2].

I changed the "no-suppression" name in Duy's WIP patch to "extra-all",
and "gentle" to "no-error". I think those are cleare,r and leave
things more open to future expansion, e.g. if we'd like pedantic-all.

1. https://public-inbox.org/git/CACsJy8CyB0igY365NMkswSgAi9_rf+XBOMQyJ7XW6iQxQiCEyQ@mail.gmail.com/
2. https://public-inbox.org/git/20180331164009.2264-1-avarab@gmail.com/

Nguyễn Thái Ngọc Duy (2):
  connect.c: mark die_initial_contact() NORETURN
  Makefile: detect compiler and enable more warnings in DEVELOPER=1

Ævar Arnfjörð Bjarmason (2):
  Makefile: add a DEVOPTS to suppress -Werror under DEVELOPER
  Makefile: add a DEVOPTS to get all of -Wextra

 Makefile        | 31 +++++++++++++++++++----------
 config.mak.dev  | 42 +++++++++++++++++++++++++++++++++++++++
 connect.c       |  2 +-
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 11 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

-- 
2.17.0.290.gded63e768a

Re: [PATCH v4 0/4] Make DEVELOPER more more flexible with DEVOPTS

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This is a v4 and replacement of gitster/nd/warn-more-for-devs. I'm
> sending this with Duy's blessing.
>
> The first two patches are the same, except for one trivial
> s/faimily/family/ typo fix.
>
> The third patch in gitster/nd/warn-more-for-devs ("Makefile: add
> EAGER_DEVELOPER mode") is gone, instead there's now a DEVOPTS
> option. The 3/4 and 4/4 add a way to turn off -Werror & the -Wextra
> suppression, respectively.

Looks sensible, mostly.  Will queue.  Thanks.

[PATCH v4 1/4] connect.c: mark die_initial_contact() NORETURN

[Ævar Arnfjörð Bjarmason]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

There is a series running in parallel with this one that adds code
like this

    switch (...) {
    case ...:
        die_initial_contact();
    case ...:

There is nothing wrong with this. There is no actual falling
through. But since gcc is not that smart and gcc 7.x introduces
-Wimplicit-fallthrough, it raises a false alarm in this case.

This class of warnings may be useful elsewhere, so instead of
suppressing the whole class, let's try to fix just this code. gcc is
smart enough to realize that no execution can continue after a
NORETURN function call and no longer raises the warning.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c
@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)
 	return check_ref(ref->name, flags);
 }
 
-static void die_initial_contact(int unexpected)
+static NORETURN void die_initial_contact(int unexpected)
 {
 	if (unexpected)
 		die(_("The remote end hung up upon initial contact"));
-- 
2.17.0.290.gded63e768a

[PATCH v4 2/4] Makefile: detect compiler and enable more warnings in DEVELOPER=1

[Ævar Arnfjörð Bjarmason]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

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 those warnings are
probably worth fixing instead of just suppressing in future.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Makefile        | 15 +++++---------
 config.mak.dev  | 38 +++++++++++++++++++++++++++++++++++
 detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+), 10 deletions(-)
 create mode 100644 config.mak.dev
 create mode 100755 detect-compiler

diff --git a/Makefile b/Makefile
index f181687250..3038c78325 100644
--- a/Makefile
+++ b/Makefile
@@ -441,6 +441,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 family are auto detected, but could be overridden by defining
+# COMPILER_FEATURES (see config.mak.dev)
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -449,15 +453,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)
@@ -1062,7 +1057,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 0000000000..716a14ecc7
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,38 @@
+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
+# if a function is public, there should be a prototype and the right
+# header file should be included. If not, it should be static.
+CFLAGS += -Wmissing-prototypes
+# These are disabled because we have these all over the place.
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+endif
+
+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
+# not worth fixing since newer compilers correctly stop complaining
+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)
+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 0000000000..70b754481c
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+# we get something like (this is at least true for gcc and clang)
+#
+# FreeBSD clang version 3.4.1 (tags/RELEASE...)
+get_version_line() {
+	$CC -v 2>&1 | grep ' version '
+}
+
+get_family() {
+	get_version_line | sed 's/^\(.*\) version [0-9][^ ]* .*/\1/'
+}
+
+get_version() {
+	get_version_line | sed 's/^.* version \([0-9][^ ]*\) .*/\1/'
+}
+
+print_flags() {
+	family=$1
+	version=$(get_version | cut -f 1 -d .)
+
+	# Print a feature flag not only for the current version, but also
+	# for any prior versions we encompass. This avoids needing to do
+	# numeric comparisons in make, which are awkward.
+	while test "$version" -gt 0
+	do
+		echo $family$version
+		version=$((version - 1))
+	done
+}
+
+case "$(get_family)" in
+gcc)
+	print_flags gcc
+	;;
+clang)
+	print_flags clang
+	;;
+"FreeBSD clang")
+	print_flags clang
+	;;
+"Apple LLVM")
+	print_flags clang
+	;;
+*)
+	: unknown compiler family
+	;;
+esac
-- 
2.17.0.290.gded63e768a

[PATCH v4 3/4] Makefile: add a DEVOPTS to suppress -Werror under DEVELOPER

[Ævar Arnfjörð Bjarmason]

Add a DEVOPTS variable that'll be used to tweak the behavior of
DEVELOPER.

I've long wanted to use DEVELOPER=1 in my production builds, but on
some old systems I still get warnings, and thus the build would
fail. However if the build/tests fail for some other reason, it would
still be useful to scroll up and see what the relevant code is warning
about.

This change allows for that. Now setting DEVELOPER will set -Werror as
before, but if DEVOPTS=no-error is provided is set you'll get the same
warnings, but without -Werror.

Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile       | 10 ++++++++++
 config.mak.dev |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/Makefile b/Makefile
index 3038c78325..9a57495fae 100644
--- a/Makefile
+++ b/Makefile
@@ -445,6 +445,16 @@ all::
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and family are auto detected, but could be overridden by defining
 # COMPILER_FEATURES (see config.mak.dev)
+#
+# When DEVELOPER is set, DEVOPTS can be used to control compiler
+# options.  This variable contains keywords separated by
+# whitespace. The following keywords are are recognized:
+#
+#    no-error:
+#
+#        suppresses the -Werror that implicitly comes with
+#        DEVELOPER=1. Useful for getting the full set of errors
+#        without immediately dying, or for logging them.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
diff --git a/config.mak.dev b/config.mak.dev
index 716a14ecc7..7a54426d78 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,4 +1,6 @@
+ifeq ($(filter no-error,$(DEVOPTS)),)
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
-- 
2.17.0.290.gded63e768a

[PATCH v4 4/4] Makefile: add a DEVOPTS to get all of -Wextra

[Ævar Arnfjörð Bjarmason]

Change DEVOPTS to understand a "extra-all" option. When the DEVELOPER
flag is enabled we turn on -Wextra, but manually switch some of the
warnings it turns on off.

This is because we have many existing occurrences of them in the code
base. This mode will stop the suppression, let the developer see and
decide whether to  fix them.

This change is a slight alteration of Nguyễn Thái Ngọc Duy
EAGER_DEVELOPER mode patch[1]

1. "[PATCH v3 3/3] Makefile: add EAGER_DEVELOPER
    mode" (<20180329150322.10722-4-pclouds@gmail.com>;
    https://public-inbox.org/git/20180329150322.10722-4-pclouds@gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile       | 6 ++++++
 config.mak.dev | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index 9a57495fae..47ddc85aae 100644
--- a/Makefile
+++ b/Makefile
@@ -455,6 +455,12 @@ all::
 #        suppresses the -Werror that implicitly comes with
 #        DEVELOPER=1. Useful for getting the full set of errors
 #        without immediately dying, or for logging them.
+#
+#    extra-all:
+#
+#        The DEVELOPER mode enables -Wextra with a few exceptions. By
+#        setting this flag the exceptions are removed, and all of
+#        -Wextra is used.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
diff --git a/config.mak.dev b/config.mak.dev
index 7a54426d78..2d244ca470 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -23,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifeq ($(filter extra-all,$(DEVOPTS)),)
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
@@ -30,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining
-- 
2.17.0.290.gded63e768a

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

[Jeff King]

On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:

> >> Why isn't this just turning on DEVELOPER=1 if we know we have a capable
> >> compiler?
> > 
> > DEVELOPER=1 is always set even before this patch. It's set and
> > exported in lib-travisci.sh.
> 
> I interpreted Peff's comment like this:
> 
> If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, 
> then we could set your additional flags in the makefile.
> 
> This way every developer with a new compiler would run these
> flags locally (if DEVELOPER=1 is set).

Actually, I was mostly just confused, and didn't realize that these were
going above and beyond what's in DEVELOPER.

But that said, now that I understand, I agree completely with your
suggestion. :)

-Peff