{"thread":{"id":"41514","subject":"[PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","startedAt":"2016-02-25T08:42:22Z","lastAt":"2017-05-10T04:59:21Z","messageCount":12,"participants":["larsxschneider@gmail.com","Matthieu Moy","Michael Haggerty","Junio C Hamano","Duy Nguyen","Lars Schneider","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"279293","messageId":"1456389742-48052-1-git-send-email-larsxschneider@gmail.com","threadId":"41514","inReplyTo":null,"subject":"[PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-25T08:42:22Z","receivedAt":"2016-02-25T08:42:22Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nWe assume Git developers have a reasonably modern compiler and recommend\nthem to enable the DEVELOPER makefile knob to ensure their patches are\nclear of all compiler warnings the Git core project cares about.\n\nEnable the DEVELOPER makefile knob in the Travis-CI build.\n\nSuggested-by: Jeff King \nSigned-off-by: Lars Schneider \n---\n\nThis patch is the successor of \"[PATCH v1] travis-ci: override CFLAGS properly,\nadd -Wdeclaration-after-statement\" [1] which enables compiler warnings for the\nTravis-CI build.\n\nPeff suggested to codify the knowledge about the compiler warnings the Git\nproject cares about [2] which I have done here.\n\nThe only problem is the \"-Wold-style-declaration\" compiler warning as this is\nonly supported by GCC and not Clang. Should we ignore that warning or would you\nprefer to detect the GCC compiler and add the warning? The Linux kernel project\ndoes a similar thing [3].\n\n\nThanks,\nLars\n\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/285752\n[2] http://article.gmane.org/gmane.comp.version-control.git/285761\n[3] https://github.com/torvalds/linux/blob/6dc390ad61ac8dfca5fa9b0823981fb6f7ec17a0/Makefile#L303-L306\n\n\n .travis.yml | 2 +-\n Documentation/CodingGuidelines | 4 ++++\n Makefile | 12 ++++++++++++\n 3 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex c3bf9c6..168ae21 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -15,12 +15,12 @@ addons:\n\n env:\n global:\n+ - DEVELOPER=1\n - P4_VERSION=\"15.2\"\n - GIT_LFS_VERSION=\"1.1.0\"\n - DEFAULT_TEST_TARGET=prove\n - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n - GIT_TEST_OPTS=\"--verbose --tee\"\n- - CFLAGS=\"-g -O2 -Wall -Werror\"\n - GIT_TEST_CLONE_2GB=YesPlease\n # t9810 occasionally fails on Travis CI OS X\n # t9816 occasionally fails with \"TAP out of sequence errors\" on Travis CI OS X\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex c6e536f..1c676c2 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -171,6 +171,10 @@ For C programs:\n\n - We try to keep to at most 80 characters per line.\n\n+ - As a Git developer we assume you have a reasonably modern compiler\n+ and we recommend you to enable the DEVELOPER makefile knob to\n+ ensure your patch is clear of all compiler warnings we care about.\n+\n - We try to support a wide range of C compilers to compile Git with,\n including old ones. That means that you should not use C99\n initializers, even if a lot of compilers grok it.\ndiff --git a/Makefile b/Makefile\nindex fd19b54..9d4614d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -380,6 +380,18 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n STRIP ?= strip\n\n+ifdef DEVELOPER\n+\tCFLAGS +=\t-Werror \\\n+\t\t\t\t-Wdeclaration-after-statement \\\n+\t\t\t\t-Wno-format-zero-length \\\n+\t\t\t\t-Wold-style-definition \\\n+\t\t\t\t-Woverflow \\\n+\t\t\t\t-Wpointer-arith \\\n+\t\t\t\t-Wstrict-prototypes \\\n+\t\t\t\t-Wunused \\\n+\t\t\t\t-Wvla\n+endif\n+\n # Create as necessary, replace existing, make ranlib unneeded.\n ARFLAGS = rcs\n\n--\n2.5.1\n"},{"id":"279304","messageId":"vpqk2lt5ef9.fsf@anie.imag.fr","threadId":"41514","inReplyTo":"1456389742-48052-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2016-02-25T09:26:18Z","receivedAt":"2016-02-25T09:26:18Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"larsxschneider@gmail.com writes:\n\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -380,6 +380,18 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n> ALL_LDFLAGS = $(LDFLAGS)\n> STRIP ?= strip\n>\n> +ifdef DEVELOPER\n> +\tCFLAGS +=\t-Werror \\\n> +\t\t\t\t-Wdeclaration-after-statement \\\n> +\t\t\t\t-Wno-format-zero-length \\\n> +\t\t\t\t-Wold-style-definition \\\n> +\t\t\t\t-Woverflow \\\n> +\t\t\t\t-Wpointer-arith \\\n> +\t\t\t\t-Wstrict-prototypes \\\n> +\t\t\t\t-Wunused \\\n> +\t\t\t\t-Wvla\n> +endif\n> +\n\nI guess you have tab-width=4. This portion looks ugly with tab-width=8.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"279309","messageId":"56CED56D.5040202@alum.mit.edu","threadId":"41514","inReplyTo":"1456389742-48052-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-02-25T10:20:29Z","receivedAt":"2016-02-25T10:20:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/25/2016 09:42 AM, larsxschneider@gmail.com wrote:\n> From: Lars Schneider \n> \n> We assume Git developers have a reasonably modern compiler and recommend\n> them to enable the DEVELOPER makefile knob to ensure their patches are\n> clear of all compiler warnings the Git core project cares about.\n> \n> Enable the DEVELOPER makefile knob in the Travis-CI build.\n> \n> Suggested-by: Jeff King \n> Signed-off-by: Lars Schneider \n> ---\n> \n> This patch is the successor of \"[PATCH v1] travis-ci: override CFLAGS properly,\n> add -Wdeclaration-after-statement\" [1] which enables compiler warnings for the\n> Travis-CI build.\n> \n> Peff suggested to codify the knowledge about the compiler warnings the Git\n> project cares about [2] which I have done here.\n> \n> The only problem is the \"-Wold-style-declaration\" compiler warning as this is\n> only supported by GCC and not Clang. Should we ignore that warning or would you\n> prefer to detect the GCC compiler and add the warning? The Linux kernel project\n> does a similar thing [3].\n> \n> \n> Thanks,\n> Lars\n> \n> \n> [1] http://thread.gmane.org/gmane.comp.version-control.git/285752\n> [2] http://article.gmane.org/gmane.comp.version-control.git/285761\n> [3] https://github.com/torvalds/linux/blob/6dc390ad61ac8dfca5fa9b0823981fb6f7ec17a0/Makefile#L303-L306\n> \n> \n> .travis.yml | 2 +-\n> Documentation/CodingGuidelines | 4 ++++\n> Makefile | 12 ++++++++++++\n> 3 files changed, 17 insertions(+), 1 deletion(-)\n> \n> [...]\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index c6e536f..1c676c2 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -171,6 +171,10 @@ For C programs:\n> \n> - We try to keep to at most 80 characters per line.\n> \n> + - As a Git developer we assume you have a reasonably modern compiler\n> + and we recommend you to enable the DEVELOPER makefile knob to\n> + ensure your patch is clear of all compiler warnings we care about.\n> +\n\nInstead of saying \"enable the DEVELOPER makefile knob\" could you be more\nexplicit? Like maybe \"create a file called `config.mak` and add the line\n`DEVELOPER=1` to it\"? (Or whatever is your preferred way to tweak this\nsetting.)\n\n> - We try to support a wide range of C compilers to compile Git with,\n> including old ones. That means that you should not use C99\n> initializers, even if a lot of compilers grok it.\n> [...]\n\nMichael\n"},{"id":"279352","messageId":"xmqqd1rkpu2z.fsf@gitster.mtv.corp.google.com","threadId":"41514","inReplyTo":"1456389742-48052-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-25T17:40:04Z","receivedAt":"2016-02-25T17:40:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Perhaps squash these two while queuing to address comments from you two?\n\nThanks.\n\n Documentation/CodingGuidelines | 3 ++-\n Makefile | 18 +++++++++---------\n 2 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 1c676c2..0ddd368 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -173,7 +173,8 @@ For C programs:\n \n - As a Git developer we assume you have a reasonably modern compiler\n and we recommend you to enable the DEVELOPER makefile knob to\n- ensure your patch is clear of all compiler warnings we care about.\n+ ensure your patch is clear of all compiler warnings we care about,\n+ by e.g. \"echo DEVELOPER=1 >>config.mak\".\n \n - We try to support a wide range of C compilers to compile Git with,\n including old ones. That means that you should not use C99\ndiff --git a/Makefile b/Makefile\nindex 9eb4032..7dc5b88 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -381,15 +381,15 @@ ALL_LDFLAGS = $(LDFLAGS)\n STRIP ?= strip\n \n ifdef DEVELOPER\n-\tCFLAGS +=\t-Werror \\\n-\t\t\t\t-Wdeclaration-after-statement \\\n-\t\t\t\t-Wno-format-zero-length \\\n-\t\t\t\t-Wold-style-definition \\\n-\t\t\t\t-Woverflow \\\n-\t\t\t\t-Wpointer-arith \\\n-\t\t\t\t-Wstrict-prototypes \\\n-\t\t\t\t-Wunused \\\n-\t\t\t\t-Wvla\n+CFLAGS += -Werror \\\n+\t-Wdeclaration-after-statement \\\n+\t-Wno-format-zero-length \\\n+\t-Wold-style-definition \\\n+\t-Woverflow \\\n+\t-Wpointer-arith \\\n+\t-Wstrict-prototypes \\\n+\t-Wunused \\\n+\t-Wvla\n endif\n \n # Create as necessary, replace existing, make ranlib unneeded.\n"},{"id":"279504","messageId":"CACsJy8CjOkNuEg=QRTATQLkYzMxpf5zryMXERZu6Zn59VDUB_Q@mail.gmail.com","threadId":"41514","inReplyTo":"1456389742-48052-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-26T09:26:04Z","receivedAt":"2016-02-26T09:26:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 25, 2016 at 3:42 PM, wrote:\n> +ifdef DEVELOPER\n> + CFLAGS += -Werror \\\n> + -Wdeclaration-after-statement \\\n> + -Wno-format-zero-length \\\n> + -Wold-style-definition \\\n> + -Woverflow \\\n> + -Wpointer-arith \\\n> + -Wstrict-prototypes \\\n> + -Wunused \\\n> + -Wvla\n\nWith the exception of $(SCRIPTS) in Makefile, I think we prefer to\navoid \\ and have one addition per statement\n\n> +endif\n-- \nDuy\n"},{"id":"279505","messageId":"F2BED69A-A595-40D5-9A03-4FCE2E67195D@gmail.com","threadId":"41514","inReplyTo":"xmqqd1rkpu2z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-26T09:26:54Z","receivedAt":"2016-02-26T09:26:54Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Thanks for the reviews and the fix :-)\n\nAny thought on the \"-Wold-style-declaration\" problem mentioned in my first v2 email?\n\n- Lars\n\n> On 25 Feb 2016, at 18:40, Junio C Hamano wrote:\n> \n> Perhaps squash these two while queuing to address comments from you two?\n> \n> Thanks.\n> \n> Documentation/CodingGuidelines | 3 ++-\n> Makefile | 18 +++++++++---------\n> 2 files changed, 11 insertions(+), 10 deletions(-)\n> \n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 1c676c2..0ddd368 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -173,7 +173,8 @@ For C programs:\n> \n> - As a Git developer we assume you have a reasonably modern compiler\n> and we recommend you to enable the DEVELOPER makefile knob to\n> - ensure your patch is clear of all compiler warnings we care about.\n> + ensure your patch is clear of all compiler warnings we care about,\n> + by e.g. \"echo DEVELOPER=1 >>config.mak\".\n> \n> - We try to support a wide range of C compilers to compile Git with,\n> including old ones. That means that you should not use C99\n> diff --git a/Makefile b/Makefile\n> index 9eb4032..7dc5b88 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -381,15 +381,15 @@ ALL_LDFLAGS = $(LDFLAGS)\n> STRIP ?= strip\n> \n> ifdef DEVELOPER\n> -\tCFLAGS +=\t-Werror \\\n> -\t\t\t\t-Wdeclaration-after-statement \\\n> -\t\t\t\t-Wno-format-zero-length \\\n> -\t\t\t\t-Wold-style-definition \\\n> -\t\t\t\t-Woverflow \\\n> -\t\t\t\t-Wpointer-arith \\\n> -\t\t\t\t-Wstrict-prototypes \\\n> -\t\t\t\t-Wunused \\\n> -\t\t\t\t-Wvla\n> +CFLAGS += -Werror \\\n> +\t-Wdeclaration-after-statement \\\n> +\t-Wno-format-zero-length \\\n> +\t-Wold-style-definition \\\n> +\t-Woverflow \\\n> +\t-Wpointer-arith \\\n> +\t-Wstrict-prototypes \\\n> +\t-Wunused \\\n> +\t-Wvla\n> endif\n> \n> # Create as necessary, replace existing, make ranlib unneeded.\n"},{"id":"279506","messageId":"17AFC0E6-FC83-4BD4-9C19-43A6B9103866@gmail.com","threadId":"41514","inReplyTo":"CACsJy8CjOkNuEg=QRTATQLkYzMxpf5zryMXERZu6Zn59VDUB_Q@mail.gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-26T09:30:15Z","receivedAt":"2016-02-26T09:30:15Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 26 Feb 2016, at 10:26, Duy Nguyen wrote:\n> \n> On Thu, Feb 25, 2016 at 3:42 PM, wrote:\n>> +ifdef DEVELOPER\n>> + CFLAGS += -Werror \\\n>> + -Wdeclaration-after-statement \\\n>> + -Wno-format-zero-length \\\n>> + -Wold-style-definition \\\n>> + -Woverflow \\\n>> + -Wpointer-arith \\\n>> + -Wstrict-prototypes \\\n>> + -Wunused \\\n>> + -Wvla\n> \n> With the exception of $(SCRIPTS) in Makefile, I think we prefer to\n> avoid \\ and have one addition per statement\n> \nI guessed that because I actually looked through the makefile to find how you deal with line \nbrakes. The problem here was that this line gets really long and then it is hard to see what \nwarnings are enabled.\nWould you be OK with using \\ for readability here?\n\nThanks,\nLars"},{"id":"279507","messageId":"CACsJy8Dc1dHp7kGgz_pY1uoXum-k3gnzD0AHFsaLQzuZihUKcw@mail.gmail.com","threadId":"41514","inReplyTo":"17AFC0E6-FC83-4BD4-9C19-43A6B9103866@gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-26T09:33:44Z","receivedAt":"2016-02-26T09:33:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 26, 2016 at 4:30 PM, Lars Schneider\n wrote:\n>\n>> On 26 Feb 2016, at 10:26, Duy Nguyen wrote:\n>>\n>> On Thu, Feb 25, 2016 at 3:42 PM, wrote:\n>>> +ifdef DEVELOPER\n>>> + CFLAGS += -Werror \\\n>>> + -Wdeclaration-after-statement \\\n>>> + -Wno-format-zero-length \\\n>>> + -Wold-style-definition \\\n>>> + -Woverflow \\\n>>> + -Wpointer-arith \\\n>>> + -Wstrict-prototypes \\\n>>> + -Wunused \\\n>>> + -Wvla\n>>\n>> With the exception of $(SCRIPTS) in Makefile, I think we prefer to\n>> avoid \\ and have one addition per statement\n>>\n> I guessed that because I actually looked through the makefile to find how you deal with line\n> brakes. The problem here was that this line gets really long and then it is hard to see what\n> warnings are enabled.\n> Would you be OK with using \\ for readability here?\n\nProbably misunderstanding. I meant something like this\n\nCFLAGS += -Werror\nCFLAGS += -Wdecl..\nCFLAGS += -Wno-form..\n-- \nDuy\n"},{"id":"279706","messageId":"47DA71B9-5A00-48E4-B7A8-6C678F597627@gmail.com","threadId":"41514","inReplyTo":"CACsJy8Dc1dHp7kGgz_pY1uoXum-k3gnzD0AHFsaLQzuZihUKcw@mail.gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-28T10:35:42Z","receivedAt":"2016-02-28T10:35:42Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 26 Feb 2016, at 10:33, Duy Nguyen wrote:\n\n> On Fri, Feb 26, 2016 at 4:30 PM, Lars Schneider\n> wrote:\n>> \n>>> On 26 Feb 2016, at 10:26, Duy Nguyen wrote:\n>>> \n>>> On Thu, Feb 25, 2016 at 3:42 PM, wrote:\n>>>> +ifdef DEVELOPER\n>>>> + CFLAGS += -Werror \\\n>>>> + -Wdeclaration-after-statement \\\n>>>> + -Wno-format-zero-length \\\n>>>> + -Wold-style-definition \\\n>>>> + -Woverflow \\\n>>>> + -Wpointer-arith \\\n>>>> + -Wstrict-prototypes \\\n>>>> + -Wunused \\\n>>>> + -Wvla\n>>> \n>>> With the exception of $(SCRIPTS) in Makefile, I think we prefer to\n>>> avoid \\ and have one addition per statement\n>>> \n>> I guessed that because I actually looked through the makefile to find how you deal with line\n>> brakes. The problem here was that this line gets really long and then it is hard to see what\n>> warnings are enabled.\n>> Would you be OK with using \\ for readability here?\n> \n> Probably misunderstanding. I meant something like this\n> \n> CFLAGS += -Werror\n> CFLAGS += -Wdecl..\n> CFLAGS += -Wno-form..\n\nOh. I just realized the patch made it already into master. Do you think I should submit\na follow up patch? I also wonder, do you see an advantage of one style over the other\nor do you want me to change it for consistency?\n\nThanks,\nLars"},{"id":"279708","messageId":"CACsJy8BA49owewikfa_LpMQ5A+2DqWLtMUPck7qRrSzv+BQU_g@mail.gmail.com","threadId":"41514","inReplyTo":"47DA71B9-5A00-48E4-B7A8-6C678F597627@gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-02-28T10:49:21Z","receivedAt":"2016-02-28T10:49:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Feb 28, 2016 at 5:35 PM, Lars Schneider\n wrote:\n> On 26 Feb 2016, at 10:33, Duy Nguyen wrote:\n>> Probably misunderstanding. I meant something like this\n>>\n>> CFLAGS += -Werror\n>> CFLAGS += -Wdecl..\n>> CFLAGS += -Wno-form..\n>\n> Oh. I just realized the patch made it already into master. Do you think I should submit\n> a follow up patch?\n\nIf it's already in, it's probably not worth changing it again.\n\n> I also wonder, do you see an advantage of one style over the other or do you want me to change it for consistency?\n\nIf you add a new option at the end, then you only add one new line.\nYou need to add '\\\\' to the last line with the other format. It also\nmakes re-sorting a bit easier (but this point is not applicable here\nbecause we need some special sort order, where we ignore \"no-\" part).\n-- \nDuy\n"},{"id":"318497","messageId":"CACBZZX4=QePHqDFnxvTSJ4siNv6TVzUvhCpA91aY8APZTpGEfQ@mail.gmail.com","threadId":"41514","inReplyTo":"1456389742-48052-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-02T13:22:23Z","receivedAt":"2017-05-02T13:22:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Feb 25, 2016 at 9:42 AM, wrote:\n> From: Lars Schneider \n>\n> We assume Git developers have a reasonably modern compiler and recommend\n> them to enable the DEVELOPER makefile knob to ensure their patches are\n> clear of all compiler warnings the Git core project cares about.\n>\n> Enable the DEVELOPER makefile knob in the Travis-CI build.\n>\n> Suggested-by: Jeff King \n> Signed-off-by: Lars Schneider \n> ---\n>\n> This patch is the successor of \"[PATCH v1] travis-ci: override CFLAGS properly,\n> add -Wdeclaration-after-statement\" [1] which enables compiler warnings for the\n> Travis-CI build.\n>\n> Peff suggested to codify the knowledge about the compiler warnings the Git\n> project cares about [2] which I have done here.\n>\n> The only problem is the \"-Wold-style-declaration\" compiler warning as this is\n> only supported by GCC and not Clang. Should we ignore that warning or would you\n> prefer to detect the GCC compiler and add the warning? The Linux kernel project\n> does a similar thing [3].\n>\n>\n> Thanks,\n> Lars\n>\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/285752\n> [2] http://article.gmane.org/gmane.comp.version-control.git/285761\n> [3] https://github.com/torvalds/linux/blob/6dc390ad61ac8dfca5fa9b0823981fb6f7ec17a0/Makefile#L303-L306\n>\n>\n> .travis.yml | 2 +-\n> Documentation/CodingGuidelines | 4 ++++\n> Makefile | 12 ++++++++++++\n> 3 files changed, 17 insertions(+), 1 deletion(-)\n>\n> diff --git a/.travis.yml b/.travis.yml\n> index c3bf9c6..168ae21 100644\n> --- a/.travis.yml\n> +++ b/.travis.yml\n> @@ -15,12 +15,12 @@ addons:\n>\n> env:\n> global:\n> + - DEVELOPER=1\n> - P4_VERSION=\"15.2\"\n> - GIT_LFS_VERSION=\"1.1.0\"\n> - DEFAULT_TEST_TARGET=prove\n> - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n> - GIT_TEST_OPTS=\"--verbose --tee\"\n> - - CFLAGS=\"-g -O2 -Wall -Werror\"\n> - GIT_TEST_CLONE_2GB=YesPlease\n> # t9810 occasionally fails on Travis CI OS X\n> # t9816 occasionally fails with \"TAP out of sequence errors\" on Travis CI OS X\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index c6e536f..1c676c2 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -171,6 +171,10 @@ For C programs:\n>\n> - We try to keep to at most 80 characters per line.\n>\n> + - As a Git developer we assume you have a reasonably modern compiler\n> + and we recommend you to enable the DEVELOPER makefile knob to\n> + ensure your patch is clear of all compiler warnings we care about.\n> +\n> - We try to support a wide range of C compilers to compile Git with,\n> including old ones. That means that you should not use C99\n> initializers, even if a lot of compilers grok it.\n> diff --git a/Makefile b/Makefile\n> index fd19b54..9d4614d 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -380,6 +380,18 @@ ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n> ALL_LDFLAGS = $(LDFLAGS)\n> STRIP ?= strip\n>\n> +ifdef DEVELOPER\n> + CFLAGS += -Werror \\\n> + -Wdeclaration-after-statement \\\n> + -Wno-format-zero-length \\\n> + -Wold-style-definition \\\n> + -Woverflow \\\n> + -Wpointer-arith \\\n> + -Wstrict-prototypes \\\n> + -Wunused \\\n> + -Wvla\n> +endif\n> +\n> # Create as necessary, replace existing, make ranlib unneeded.\n> ARFLAGS = rcs\n>\n> --\n> 2.5.1\n\n[I realize this patch has long since been integrated]\n\nIs there any way with this to both supply CFLAGS & DEVELOPER=1 on the\ncommand-line, to get my custom -O & these -W flags? I.e.:\n\n$ make DEVELOPER=1 V=1\n[...] -g -O2 -Wall -Werror -Wdeclaration-after-statement\n-Wno-format-zero-length -Wold-style-definition -Woverflow\n-Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -I. [...]\n$ make DEVELOPER=1 CFLAGS=\"-g -O0 -Wall\" V=1\n[...] -g -O0 -Wall -I. [...]\n\nI thought the second case would prepend my \"-g -O0 -Wall\" but then be\nfollowed by the various -W developer flags, but it isn't, am I just\ndoing something stupid, or is there no way to combine these two?\n"},{"id":"319251","messageId":"20170510045914.adtb5qmnc53z4vdq@sigill.intra.peff.net","threadId":"41514","inReplyTo":"CACBZZX4=QePHqDFnxvTSJ4siNv6TVzUvhCpA91aY8APZTpGEfQ@mail.gmail.com","subject":"Re: [PATCH v2] add DEVELOPER makefile knob to check for acknowledged warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-10T04:59:15Z","receivedAt":"2017-05-10T04:59:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 02, 2017 at 03:22:23PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Is there any way with this to both supply CFLAGS & DEVELOPER=1 on the\n> command-line, to get my custom -O & these -W flags? I.e.:\n> \n> $ make DEVELOPER=1 V=1\n> [...] -g -O2 -Wall -Werror -Wdeclaration-after-statement\n> -Wno-format-zero-length -Wold-style-definition -Woverflow\n> -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -I. [...]\n> $ make DEVELOPER=1 CFLAGS=\"-g -O0 -Wall\" V=1\n> [...] -g -O0 -Wall -I. [...]\n> \n> I thought the second case would prepend my \"-g -O0 -Wall\" but then be\n> followed by the various -W developer flags, but it isn't, am I just\n> doing something stupid, or is there no way to combine these two?\n\nThe problem is that when you give \"make\" a variable on the command line,\nit overrides all of the modifications. So if you were to set that CFLAGS\nin your config.mak, I think everything would work as you expect.\n\nI actually do this in my config.mak:\n\n O = 0\n CFLAGS += -g -O$(O)\n\nwhich lets me override the optimization level as a one-off on the\ncommand-line:\n\n make O=2\n\nwithout disturbing the rest of the CFLAGS. I also do this:\n\n CFLAGS += $(EXTRA_CFLAGS)\n\nas a catch-all, so that I can do:\n\n make EXTRA_CFLAGS=-Wone-off-warning-that-I-am-testing\n\nPerhaps those are things the main Makefile should support. I dunno. You\ncould see the full depths of my depravity at:\n\n https://github.com/peff/git/blame/meta/config/config.mak\n\n(I linked to the blame view because there are a lot of WTF bits in there\nthat are explained by the commit messages).\n\n-Peff\n"}]}