v2add DEVELOPER makefile knob to check for acknowledged warnings

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

> From: Lars Schneider <larsxschneider@gmail.com>
> 
> 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 <peff@peff.net>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> 
> 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/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.
> +

> On 25 Feb 2016, at 18:40, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Perhaps squash these two while queuing to address comments from you two?
> 
> Thanks.
> 
> Documentation/CodingGuidelines |  3 ++-
> Makefile                       | 18 +++++++++---------
> 2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 1c676c2..0ddd368 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -173,7 +173,8 @@ For C programs:
> 
>  - 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.
> +   ensure your patch is clear of all compiler warnings we care about,
> +   by e.g. "echo DEVELOPER=1 >>config.mak".
> 
>  - 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
> diff --git a/Makefile b/Makefile
> index 9eb4032..7dc5b88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -381,15 +381,15 @@ 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
> +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.

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

> On 26 Feb 2016, at 10:26, Duy Nguyen <pclouds@gmail.com> wrote:
> 
> On Thu, Feb 25, 2016 at 3:42 PM,  <larsxschneider@gmail.com> 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
> 

>
>> On 26 Feb 2016, at 10:26, Duy Nguyen <pclouds@gmail.com> wrote:
>>
>> On Thu, Feb 25, 2016 at 3:42 PM,  <larsxschneider@gmail.com> 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
>>
> 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?

> On Fri, Feb 26, 2016 at 4:30 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> 
>>> On 26 Feb 2016, at 10:26, Duy Nguyen <pclouds@gmail.com> wrote:
>>> 
>>> On Thu, Feb 25, 2016 at 3:42 PM,  <larsxschneider@gmail.com> 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
>>> 
>> 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..

> On 26 Feb 2016, at 10:33, Duy Nguyen <pclouds@gmail.com> wrote:
>> 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?

> From: Lars Schneider <larsxschneider@gmail.com>
>
> 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 <peff@peff.net>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>
> 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

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