threads / patch / 47961

patchtravis-ci: enable more warnings on travis linux-gcc job

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

## tl;dr

47 messages between Mar 3, 2018 and Apr 16, 2018. Diffs are folded; open one to read it.

replies: 46people: 7as markdown or json

Nguyễn Thái Ngọc Duy· Mar 3, 2018, 01:46 UTC · lore

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(+)
Show changes to 2 files +18 −0

.travis.yml, ci/run-build.sh

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
Nguyễn Thái Ngọc Duy· Mar 16, 2018, 19:33 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(+)
Show changes to 2 files +19 −0

.travis.yml, ci/run-build-and-tests.sh

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
Jeff King· Mar 16, 2018, 21:22 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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

Show 23 quoted lines
> 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
Duy Nguyen· Mar 17, 2018, 08:01 UTC · re: Jeff King · lore

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

On Fri, Mar 16, 2018 at 10:22 PM, Jeff King <peff@peff.net> wrote:
Show 26 quoted lines
>> 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
Lars Schneider· Mar 17, 2018, 14:29 UTC · re: Duy Nguyen · lore

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

Show 32 quoted lines
> 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
Duy Nguyen· Mar 17, 2018, 14:59 UTC · re: Lars Schneider · lore

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

On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:
Show 7 quoted lines
> 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< --
Show changes to 3 files +28 −11

.travis.yml, Makefile, config.mak.dev

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< --
Jeff King· Mar 17, 2018, 16:08 UTC · re: Duy Nguyen · lore

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

On Sat, Mar 17, 2018 at 03:59:23PM +0100, Duy Nguyen wrote:
Show 11 quoted lines
> 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?

Show changes to 3 files +61 −10

Makefile, config.mak.dev, 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..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
Jeff King· Mar 17, 2018, 16:12 UTC · re: Jeff King · lore

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

On Sat, Mar 17, 2018 at 12:08:32PM -0400, Jeff King wrote:
Show 11 quoted lines
> +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
Junio C Hamano· Mar 17, 2018, 23:50 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> 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.
Nguyễn Thái Ngọc Duy· Mar 18, 2018, 08:18 UTC · re: Jeff King · lore

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

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
Show changes to 3 files +79 −10

Makefile, config.mak.dev, 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
Eric Sunshine· Mar 18, 2018, 09:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:
Show 15 quoted lines
> 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".
Show 14 quoted lines
> +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 ---
Show changes to detect-compiler +3 −0
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 ---
Duy Nguyen· Mar 18, 2018, 09:17 UTC · re: Eric Sunshine · lore

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

On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 25 quoted lines
> 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
Jeff King· Mar 18, 2018, 09:20 UTC · re: Duy Nguyen · lore

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

On Sun, Mar 18, 2018 at 10:17:41AM +0100, Duy Nguyen wrote:
Show 31 quoted lines
> 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
Eric Sunshine· Mar 18, 2018, 09:24 UTC · re: Duy Nguyen · lore

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

On Sun, Mar 18, 2018 at 5:17 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 13 quoted lines
> 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.)

Jeff King· Mar 18, 2018, 09:28 UTC · re: Eric Sunshine · lore

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

On Sun, Mar 18, 2018 at 05:06:07AM -0400, Eric Sunshine wrote:
Show 8 quoted lines
> 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
Eric Sunshine· Mar 18, 2018, 09:37 UTC · re: Jeff King · lore

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

On Sun, Mar 18, 2018 at 5:28 AM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
> 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
Jeff King· Mar 18, 2018, 09:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Duy Nguyen· Mar 18, 2018, 09:45 UTC · re: Jeff King · lore

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

On Sun, Mar 18, 2018 at 10:26 AM, Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> 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
Duy Nguyen· Mar 18, 2018, 15:55 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Ramsay Jones· Mar 18, 2018, 18:56 UTC · re: Duy Nguyen · lore

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

On 18/03/18 15:55, Duy Nguyen wrote:
Show 8 quoted lines
> 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

Duy Nguyen· Mar 19, 2018, 16:30 UTC · re: Ramsay Jones · lore

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

On Sun, Mar 18, 2018 at 7:56 PM, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:

Show 18 quoted lines
>
>
> 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
Jeff King· Mar 20, 2018, 05:32 UTC · re: Duy Nguyen · lore

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

On Sun, Mar 18, 2018 at 04:55:25PM +0100, Duy Nguyen wrote:
Show 8 quoted lines
> 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.
Nguyễn Thái Ngọc Duy· Mar 24, 2018, 12:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Show changes to 3 files +88 −10

Makefile, config.mak.dev, 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
Eric Sunshine· Mar 25, 2018, 00:40 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Sat, Mar 24, 2018 at 8:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 8 quoted lines
> 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>
Junio C Hamano· Mar 26, 2018, 22:02 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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().
Duy Nguyen· Mar 27, 2018, 15:03 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> 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
Junio C Hamano· Mar 27, 2018, 16:52 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
 On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>>
>>  - 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
Nguyễn Thái Ngọc Duy· Mar 29, 2018, 15:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Show changes to 3 files +21 −4

Makefile, config.mak.dev, connect.c

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
Nguyễn Thái Ngọc Duy· Mar 29, 2018, 15:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +10 −1

Makefile, config.mak.dev

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
Nguyễn Thái Ngọc Duy· Mar 29, 2018, 15:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Show changes to 3 files +96 −10

Makefile, config.mak.dev, 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
Nguyễn Thái Ngọc Duy· Mar 29, 2018, 15:03 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to connect.c +1 −1
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
Ævar Arnfjörð Bjarmason· Mar 31, 2018, 16:40 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +19 −8

Makefile, config.mak.dev

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
Duy Nguyen· Mar 31, 2018, 18:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 18 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Apr 3, 2018, 09:19 UTC · re: Duy Nguyen · lore

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

On Sat, Mar 31 2018, Duy Nguyen wrote:
Show 27 quoted lines
> 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.

Duy Nguyen· Apr 3, 2018, 15:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Tue, Apr 03, 2018 at 11:19:46AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 33 quoted lines
> 
> 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 :)

Show 6 quoted lines
> 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< --
Show changes to 2 files +9 −1

Makefile, config.mak.dev

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< --
Jeff King· Apr 6, 2018, 21:42 UTC · re: Duy Nguyen · lore

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

On Tue, Apr 03, 2018 at 05:17:00PM +0200, Duy Nguyen wrote:
Show 10 quoted lines
> 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
Duy Nguyen· Apr 7, 2018, 09:52 UTC · re: Jeff King · lore

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

On Fri, Apr 6, 2018 at 11:42 PM, Jeff King <peff@peff.net> wrote:
Show 18 quoted lines
> 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 :)
Show 5 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Apr 7, 2018, 12:36 UTC · re: Duy Nguyen · lore

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

On Sat, Apr 07 2018, Duy Nguyen wrote:
Show 21 quoted lines
> 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).

Duy Nguyen· Apr 7, 2018, 17:03 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Ævar Arnfjörð Bjarmason· Apr 7, 2018, 18:38 UTC · re: Duy Nguyen · lore

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

On Sat, Apr 07 2018, Duy Nguyen wrote:
Show 7 quoted lines
> 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.

Ævar Arnfjörð Bjarmason· Apr 14, 2018, 19:19 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Ævar Arnfjörð Bjarmason· Apr 14, 2018, 19:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to connect.c +1 −1
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
Ævar Arnfjörð Bjarmason· Apr 14, 2018, 19:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Show changes to 3 files +96 −10

Makefile, config.mak.dev, 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
Ævar Arnfjörð Bjarmason· Apr 14, 2018, 19:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to 2 files +12 −0

Makefile, config.mak.dev

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
Ævar Arnfjörð Bjarmason· Apr 14, 2018, 19:19 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to 2 files +8 −0

Makefile, config.mak.dev

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
Junio C Hamano· Apr 16, 2018, 04:57 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 10 quoted lines
> 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.
Jeff King· Mar 17, 2018, 15:16 UTC · re: Lars Schneider · lore

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

On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:
Show 13 quoted lines
> >> 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

← back to recent threads