[PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings

From: Lars Schneider <redacted>

We assume Git developers have a reasonably modern compiler and recommend
them to enable the DEVELOPER makefile knob to ensure their patches are
clear of all compiler warnings the Git core project cares about.

Enable the DEVELOPER makefile knob in the Travis-CI build.

Suggested-by: Jeff King <redacted>
Signed-off-by: Lars Schneider <redacted>
---

This patch is the successor of "[PATCH v1] travis-ci: override CFLAGS properly,
add -Wdeclaration-after-statement" [1] which enables compiler warnings for the
Travis-CI build.

Peff suggested to codify the knowledge about the compiler warnings the Git
project cares about [2] which I have done here.

The only problem is the "-Wold-style-declaration" compiler warning as this is
only supported by GCC and not Clang. Should we ignore that warning or would you
prefer to detect the GCC compiler and add the warning? The Linux kernel project
does a similar thing [3].


Thanks,
Lars


[1] http://thread.gmane.org/gmane.comp.version-control.git/285752
[2] http://article.gmane.org/gmane.comp.version-control.git/285761
[3] https://github.com/torvalds/linux/blob/6dc390ad61ac8dfca5fa9b0823981fb6f7ec17a0/Makefile#L303-L306


 .travis.yml                    |  2 +-
 Documentation/CodingGuidelines |  4 ++++
 Makefile                       | 12 ++++++++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index c3bf9c6..168ae21 100644
--- a/.travis.yml
+++ b/.travis.yml

@@ -15,12 +15,12 @@ addons:

 env:
   global:
+    - DEVELOPER=1
     - P4_VERSION="15.2"
     - GIT_LFS_VERSION="1.1.0"
     - DEFAULT_TEST_TARGET=prove
     - GIT_PROVE_OPTS="--timer --jobs 3"
     - GIT_TEST_OPTS="--verbose --tee"
-    - CFLAGS="-g -O2 -Wall -Werror"
     - GIT_TEST_CLONE_2GB=YesPlease
     # t9810 occasionally fails on Travis CI OS X
     # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index c6e536f..1c676c2 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines

@@ -171,6 +171,10 @@ For C programs:

  - We try to keep to at most 80 characters per line.

+ - As a Git developer we assume you have a reasonably modern compiler
+   and we recommend you to enable the DEVELOPER makefile knob to
+   ensure your patch is clear of all compiler warnings we care about.
+
  - We try to support a wide range of C compilers to compile Git with,
    including old ones. That means that you should not use C99
    initializers, even if a lot of compilers grok it.

diff --git a/Makefile b/Makefile
index fd19b54..9d4614d 100644
--- a/Makefile
+++ b/Makefile

@@ -380,6 +380,18 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 STRIP ?= strip

+ifdef DEVELOPER
+	CFLAGS +=	-Werror \
+				-Wdeclaration-after-statement \
+				-Wno-format-zero-length \
+				-Wold-style-definition \
+				-Woverflow \
+				-Wpointer-arith \
+				-Wstrict-prototypes \
+				-Wunused \
+				-Wvla
+endif
+
 # Create as necessary, replace existing, make ranlib unneeded.
 ARFLAGS = rcs

--

2.5.1

On 02/25/2016 09:42 AM, larsxschneider@gmail.com wrote:

Instead of saying "enable the DEVELOPER makefile knob" could you be more
explicit? Like maybe "create a file called `config.mak` and add the line
`DEVELOPER=1` to it"? (Or whatever is your preferred way to tweak this
setting.)

  - We try to support a wide range of C compilers to compile Git with,
    including old ones. That means that you should not use C99
    initializers, even if a lot of compilers grok it.
[...]

Michael

On Thu, Feb 25, 2016 at 3:42 PM,  [off-list ref] wrote:

+ifdef DEVELOPER
+       CFLAGS +=       -Werror \
+                               -Wdeclaration-after-statement \
+                               -Wno-format-zero-length \
+                               -Wold-style-definition \
+                               -Woverflow \
+                               -Wpointer-arith \
+                               -Wstrict-prototypes \
+                               -Wunused \
+                               -Wvla

With the exception of $(SCRIPTS) in Makefile, I think we prefer to
avoid \ and have one addition per statement

+endif

-- 
Duy

On 26 Feb 2016, at 10:26, Duy Nguyen [off-list ref] wrote:

On Thu, Feb 25, 2016 at 3:42 PM,  [off-list ref] wrote:

quoted

+ifdef DEVELOPER
+       CFLAGS +=       -Werror \
+                               -Wdeclaration-after-statement \
+                               -Wno-format-zero-length \
+                               -Wold-style-definition \
+                               -Woverflow \
+                               -Wpointer-arith \
+                               -Wstrict-prototypes \
+                               -Wunused \
+                               -Wvla

With the exception of $(SCRIPTS) in Makefile, I think we prefer to
avoid \ and have one addition per statement

I guessed that because I actually looked through the makefile to find how you deal with line 
brakes. The problem here was that this line gets really long and then it is hard to see what 
warnings are enabled.
Would you be OK with using \ for readability here?

Thanks,
Lars

On Fri, Feb 26, 2016 at 4:30 PM, Lars Schneider
[off-list ref] wrote:

quoted

On 26 Feb 2016, at 10:26, Duy Nguyen [off-list ref] wrote:

On Thu, Feb 25, 2016 at 3:42 PM,  [off-list ref] wrote:

quoted

+ifdef DEVELOPER
+       CFLAGS +=       -Werror \
+                               -Wdeclaration-after-statement \
+                               -Wno-format-zero-length \
+                               -Wold-style-definition \
+                               -Woverflow \
+                               -Wpointer-arith \
+                               -Wstrict-prototypes \
+                               -Wunused \
+                               -Wvla

With the exception of $(SCRIPTS) in Makefile, I think we prefer to
avoid \ and have one addition per statement

I guessed that because I actually looked through the makefile to find how you deal with line
brakes. The problem here was that this line gets really long and then it is hard to see what
warnings are enabled.
Would you be OK with using \ for readability here?

Probably misunderstanding. I meant something like this

CFLAGS += -Werror
CFLAGS += -Wdecl..
CFLAGS += -Wno-form..
-- 
Duy

On 26 Feb 2016, at 10:33, Duy Nguyen [off-list ref] wrote:

On Fri, Feb 26, 2016 at 4:30 PM, Lars Schneider
[off-list ref] wrote:

quoted

quoted

On 26 Feb 2016, at 10:26, Duy Nguyen [off-list ref] wrote:

On Thu, Feb 25, 2016 at 3:42 PM,  [off-list ref] wrote:

quoted

+ifdef DEVELOPER
+       CFLAGS +=       -Werror \
+                               -Wdeclaration-after-statement \
+                               -Wno-format-zero-length \
+                               -Wold-style-definition \
+                               -Woverflow \
+                               -Wpointer-arith \
+                               -Wstrict-prototypes \
+                               -Wunused \
+                               -Wvla

With the exception of $(SCRIPTS) in Makefile, I think we prefer to
avoid \ and have one addition per statement

I guessed that because I actually looked through the makefile to find how you deal with line
brakes. The problem here was that this line gets really long and then it is hard to see what
warnings are enabled.
Would you be OK with using \ for readability here?

Probably misunderstanding. I meant something like this

CFLAGS += -Werror
CFLAGS += -Wdecl..
CFLAGS += -Wno-form..

Oh. I just realized the patch made it already into master. Do you think I should submit
a follow up patch? I also wonder, do you see an advantage of one style over the other
or do you want me to change it for consistency?

Thanks,
Lars

On Sun, Feb 28, 2016 at 5:35 PM, Lars Schneider
[off-list ref] wrote:

On 26 Feb 2016, at 10:33, Duy Nguyen [off-list ref] wrote:

quoted

Probably misunderstanding. I meant something like this

CFLAGS += -Werror
CFLAGS += -Wdecl..
CFLAGS += -Wno-form..

Oh. I just realized the patch made it already into master. Do you think I should submit
a follow up patch?

If it's already in, it's probably not worth changing it again.

I also wonder, do you see an advantage of one style over the other or do you want me to change it for consistency?

If you add a new option at the end, then you only add one new line.
You need to add '\\' to the last line with the other format. It also
makes re-sorting a bit easier (but this point is not applicable here
because we need some special sort order, where we ignore "no-" part).
-- 
Duy

On Thu, Feb 25, 2016 at 9:42 AM,  [off-list ref] wrote:

[I realize this patch has long since been integrated]

Is there any way with this to both supply CFLAGS & DEVELOPER=1 on the
command-line, to get my custom -O<whatever> & these -W flags? I.e.:

$ make DEVELOPER=1 V=1
[...] -g -O2 -Wall -Werror -Wdeclaration-after-statement
-Wno-format-zero-length -Wold-style-definition -Woverflow
-Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -I. [...]
$ make DEVELOPER=1 CFLAGS="-g -O0 -Wall" V=1
[...] -g -O0 -Wall -I. [...]

I thought the second case would prepend my "-g -O0 -Wall" but then be
followed by the various -W developer flags, but it isn't, am I just
doing something stupid, or is there no way to combine these two?

On Tue, May 02, 2017 at 03:22:23PM +0200, Ævar Arnfjörð Bjarmason wrote:

Is there any way with this to both supply CFLAGS & DEVELOPER=1 on the
command-line, to get my custom -O<whatever> & these -W flags? I.e.:

$ make DEVELOPER=1 V=1
[...] -g -O2 -Wall -Werror -Wdeclaration-after-statement
-Wno-format-zero-length -Wold-style-definition -Woverflow
-Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -I. [...]
$ make DEVELOPER=1 CFLAGS="-g -O0 -Wall" V=1
[...] -g -O0 -Wall -I. [...]

I thought the second case would prepend my "-g -O0 -Wall" but then be
followed by the various -W developer flags, but it isn't, am I just
doing something stupid, or is there no way to combine these two?

The problem is that when you give "make" a variable on the command line,
it overrides all of the modifications. So if you were to set that CFLAGS
in your config.mak, I think everything would work as you expect.

I actually do this in my config.mak:

  O = 0
  CFLAGS += -g -O$(O)

which lets me override the optimization level as a one-off on the
command-line:

  make O=2

without disturbing the rest of the CFLAGS. I also do this:

  CFLAGS += $(EXTRA_CFLAGS)

as a catch-all, so that I can do:

  make EXTRA_CFLAGS=-Wone-off-warning-that-I-am-testing

Perhaps those are things the main Makefile should support. I dunno. You
could see the full depths of my depravity at:

  https://github.com/peff/git/blame/meta/config/config.mak

(I linked to the blame view because there are a lot of WTF bits in there
that are explained by the commit messages).

-Peff