{"thread":{"id":"47961","subject":"[PATCH] travis-ci: enable more warnings on travis linux-gcc job","startedAt":"2018-03-03T01:46:27Z","lastAt":"2018-04-16T04:57:20Z","messageCount":47,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Duy Nguyen","Lars Schneider","Junio C Hamano","Eric Sunshine","Ramsay Jones","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"340845","messageId":"20180303014605.10603-1-pclouds@gmail.com","threadId":"47961","inReplyTo":null,"subject":"[PATCH] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-03T01:46:05Z","receivedAt":"2018-03-03T01:46:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We have DEVELOPER config to enable more warnings, but since we can't set\na fixed gcc version to all devs, that has to be a bit more conservative.\nOn travis, we know almost exact version to be used (bumped to 6.x for\nmore warnings), we could be more aggressive.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n .travis.yml | 3 +++\n ci/run-build.sh | 15 +++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex 4684b3f4f3..273b1d508a 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -16,10 +16,13 @@ compiler:\n \n addons:\n apt:\n+ sources:\n+ - ubuntu-toolchain-r-test\n packages:\n - language-pack-is\n - git-svn\n - apache2\n+ - gcc-6\n \n matrix:\n include:\ndiff --git a/ci/run-build.sh b/ci/run-build.sh\nindex 4f940d1032..04e163359c 100755\n--- a/ci/run-build.sh\n+++ b/ci/run-build.sh\n@@ -5,4 +5,19 @@\n \n . ${0%/*}/lib-travisci.sh\n \n+if [ \"$jobname\" = linux-gcc ]; then\n+\tgcc-6 --version\n+\tcat >config.mak <<-EOF\n+\tCC=gcc-6\n+\tCFLAGS = -g -O2 -Wall\n+\tCFLAGS += -Wextra\n+\tCFLAGS += -Wmissing-prototypes\n+\tCFLAGS += -Wno-empty-body\n+\tCFLAGS += -Wno-maybe-uninitialized\n+\tCFLAGS += -Wno-missing-field-initializers\n+\tCFLAGS += -Wno-sign-compare\n+\tCFLAGS += -Wno-unused-function\n+\tCFLAGS += -Wno-unused-parameter\n+\tEOF\n+fi\n make --jobs=2\n-- \n2.16.1.435.g8f24da2e1a\n\n"},{"id":"341912","messageId":"20180316193355.20161-1-pclouds@gmail.com","threadId":"47961","inReplyTo":"20180303014605.10603-1-pclouds@gmail.com","subject":"[PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-16T19:33:55Z","receivedAt":"2018-03-16T19:34:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We have DEVELOPER config to enable more warnings, but since we can't set\na fixed gcc version to all devs, that has to be a bit more conservative.\nOn travis, we know almost exact version to be used (bumped to 6.x for\nmore warnings), we could be more aggressive.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I still think it's a good thing to do. So here's the rebased version\n (v1 collides badly with another series, which has landed)\n\n .travis.yml | 3 +++\n ci/run-build-and-tests.sh | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex 5f5ee4f3bd..0b3c50f5e7 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -16,10 +16,13 @@ compiler:\n \n addons:\n apt:\n+ sources:\n+ - ubuntu-toolchain-r-test\n packages:\n - language-pack-is\n - git-svn\n - apache2\n+ - gcc-6\n \n matrix:\n include:\ndiff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\nindex 3735ce413f..f6f346c468 100755\n--- a/ci/run-build-and-tests.sh\n+++ b/ci/run-build-and-tests.sh\n@@ -7,6 +7,22 @@\n \n ln -s \"$cache_dir/.prove\" t/.prove\n \n+if [ \"$jobname\" = linux-gcc ]; then\n+\tgcc-6 --version\n+\tcat >config.mak <<-EOF\n+\tCC=gcc-6\n+\tCFLAGS = -g -O2 -Wall\n+\tCFLAGS += -Wextra\n+\tCFLAGS += -Wmissing-prototypes\n+\tCFLAGS += -Wno-empty-body\n+\tCFLAGS += -Wno-maybe-uninitialized\n+\tCFLAGS += -Wno-missing-field-initializers\n+\tCFLAGS += -Wno-sign-compare\n+\tCFLAGS += -Wno-unused-function\n+\tCFLAGS += -Wno-unused-parameter\n+\tEOF\n+fi\n+\n make --jobs=2\n make --quiet test\n if test \"$jobname\" = \"linux-gcc\"\n-- \n2.16.2.903.gd04caf5039\n\n"},{"id":"341945","messageId":"20180316212208.GC12333@sigill.intra.peff.net","threadId":"47961","inReplyTo":"20180316193355.20161-1-pclouds@gmail.com","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-16T21:22:08Z","receivedAt":"2018-03-16T21:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 16, 2018 at 08:33:55PM +0100, Nguyễn Thái Ngọc Duy wrote:\n\n> We have DEVELOPER config to enable more warnings, but since we can't set\n> a fixed gcc version to all devs, that has to be a bit more conservative.\n> On travis, we know almost exact version to be used (bumped to 6.x for\n> more warnings), we could be more aggressive.\n\nCertainly it makes sense to dial up any static checks we can for the\nTravis environment, but...\n\n> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n> index 3735ce413f..f6f346c468 100755\n> --- a/ci/run-build-and-tests.sh\n> +++ b/ci/run-build-and-tests.sh\n> @@ -7,6 +7,22 @@\n> \n> ln -s \"$cache_dir/.prove\" t/.prove\n> \n> +if [ \"$jobname\" = linux-gcc ]; then\n> +\tgcc-6 --version\n> +\tcat >config.mak <<-EOF\n> +\tCC=gcc-6\n> +\tCFLAGS = -g -O2 -Wall\n> +\tCFLAGS += -Wextra\n> +\tCFLAGS += -Wmissing-prototypes\n> +\tCFLAGS += -Wno-empty-body\n> +\tCFLAGS += -Wno-maybe-uninitialized\n> +\tCFLAGS += -Wno-missing-field-initializers\n> +\tCFLAGS += -Wno-sign-compare\n> +\tCFLAGS += -Wno-unused-function\n> +\tCFLAGS += -Wno-unused-parameter\n> +\tEOF\n> +fi\n\nWhy isn't this just turning on DEVELOPER=1 if we know we have a capable\ncompiler?\n\n-Peff\n"},{"id":"342010","messageId":"CACsJy8A3_itjs0MRar+Aog=0yZDuAK2-x4MU=Rvqh2C6Le-7Aw@mail.gmail.com","threadId":"47961","inReplyTo":"20180316212208.GC12333@sigill.intra.peff.net","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-17T08:01:31Z","receivedAt":"2018-03-17T08:02:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 16, 2018 at 10:22 PM, Jeff King wrote:\n>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n>> index 3735ce413f..f6f346c468 100755\n>> --- a/ci/run-build-and-tests.sh\n>> +++ b/ci/run-build-and-tests.sh\n>> @@ -7,6 +7,22 @@\n>>\n>> ln -s \"$cache_dir/.prove\" t/.prove\n>>\n>> +if [ \"$jobname\" = linux-gcc ]; then\n>> + gcc-6 --version\n>> + cat >config.mak <<-EOF\n>> + CC=gcc-6\n>> + CFLAGS = -g -O2 -Wall\n>> + CFLAGS += -Wextra\n>> + CFLAGS += -Wmissing-prototypes\n>> + CFLAGS += -Wno-empty-body\n>> + CFLAGS += -Wno-maybe-uninitialized\n>> + CFLAGS += -Wno-missing-field-initializers\n>> + CFLAGS += -Wno-sign-compare\n>> + CFLAGS += -Wno-unused-function\n>> + CFLAGS += -Wno-unused-parameter\n>> + EOF\n>> +fi\n>\n> Why isn't this just turning on DEVELOPER=1 if we know we have a capable\n> compiler?\n\nDEVELOPER=1 is always set even before this patch. It's set and\nexported in lib-travisci.sh.\n-- \nDuy\n"},{"id":"342031","messageId":"D8137CD9-9E11-4FBC-B89B-907C007FC6E9@gmail.com","threadId":"47961","inReplyTo":"CACsJy8A3_itjs0MRar+Aog=0yZDuAK2-x4MU=Rvqh2C6Le-7Aw@mail.gmail.com","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2018-03-17T14:29:31Z","receivedAt":"2018-03-17T14:29:39Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 17 Mar 2018, at 09:01, Duy Nguyen wrote:\n> \n> On Fri, Mar 16, 2018 at 10:22 PM, Jeff King wrote:\n>>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh\n>>> index 3735ce413f..f6f346c468 100755\n>>> --- a/ci/run-build-and-tests.sh\n>>> +++ b/ci/run-build-and-tests.sh\n>>> @@ -7,6 +7,22 @@\n>>> \n>>> ln -s \"$cache_dir/.prove\" t/.prove\n>>> \n>>> +if [ \"$jobname\" = linux-gcc ]; then\n>>> + gcc-6 --version\n>>> + cat >config.mak <<-EOF\n>>> + CC=gcc-6\n>>> + CFLAGS = -g -O2 -Wall\n>>> + CFLAGS += -Wextra\n>>> + CFLAGS += -Wmissing-prototypes\n>>> + CFLAGS += -Wno-empty-body\n>>> + CFLAGS += -Wno-maybe-uninitialized\n>>> + CFLAGS += -Wno-missing-field-initializers\n>>> + CFLAGS += -Wno-sign-compare\n>>> + CFLAGS += -Wno-unused-function\n>>> + CFLAGS += -Wno-unused-parameter\n>>> + EOF\n>>> +fi\n>> \n>> Why isn't this just turning on DEVELOPER=1 if we know we have a capable\n>> compiler?\n> \n> DEVELOPER=1 is always set even before this patch. It's set and\n> exported in lib-travisci.sh.\n\nI interpreted Peff's comment like this:\n\nIf DEVELOPER=1 is set and we detect a gcc-6 in the makefile, \nthen we could set your additional flags in the makefile.\n\nThis way every developer with a new compiler would run these\nflags locally (if DEVELOPER=1 is set).\n\n- Lars\n"},{"id":"342032","messageId":"20180317145923.GA24689@duynguyen.home","threadId":"47961","inReplyTo":"D8137CD9-9E11-4FBC-B89B-907C007FC6E9@gmail.com","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-17T14:59:23Z","receivedAt":"2018-03-17T14:59:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:\n> I interpreted Peff's comment like this:\n> \n> If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, \n> then we could set your additional flags in the makefile.\n> \n> This way every developer with a new compiler would run these\n> flags locally (if DEVELOPER=1 is set).\n\nAha. Something like this? I split developer cflags out to a separate\nfile because I imagine people may start to add gcc7, clang....\n\n-- 8< --\ndiff --git a/.travis.yml b/.travis.yml\nindex 5f5ee4f3bd..0b3c50f5e7 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -16,10 +16,13 @@ compiler:\n \n addons:\n apt:\n+ sources:\n+ - ubuntu-toolchain-r-test\n packages:\n - language-pack-is\n - git-svn\n - apache2\n+ - gcc-6\n \n matrix:\n include:\ndiff --git a/Makefile b/Makefile\nindex de4b8f0c02..e8321e44d6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -436,15 +436,6 @@ GIT-VERSION-FILE: FORCE\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n-DEVELOPER_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 LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -1045,7 +1036,8 @@ include config.mak.uname\n -include config.mak\n \n ifdef DEVELOPER\n-CFLAGS += $(DEVELOPER_CFLAGS)\n+include config.mak.dev\n+CFLAGS += $(DEV_CFLAGS)\n endif\n \n comma := ,\ndiff --git a/config.mak.dev b/config.mak.dev\nindex e69de29bb2..644d0d581f 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -0,0 +1,23 @@\n+DEV_CFLAGS =\n+DEV_CFLAGS += -Werror\n+DEV_CFLAGS += -Wdeclaration-after-statement\n+DEV_CFLAGS += -Wno-format-zero-length\n+DEV_CFLAGS += -Wold-style-definition\n+DEV_CFLAGS += -Woverflow\n+DEV_CFLAGS += -Wpointer-arith\n+DEV_CFLAGS += -Wstrict-prototypes\n+DEV_CFLAGS += -Wunused\n+DEV_CFLAGS += -Wvla\n+\n+GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) -dumpversion | cut -f 1 -d .')\n+\n+ifeq ($(GCC_VER),6)\n+DEV_CFLAGS += -Wextra\n+DEV_CFLAGS += -Wmissing-prototypes\n+DEV_CFLAGS += -Wno-empty-body\n+DEV_CFLAGS += -Wno-maybe-uninitialized\n+DEV_CFLAGS += -Wno-missing-field-initializers\n+DEV_CFLAGS += -Wno-sign-compare\n+DEV_CFLAGS += -Wno-unused-function\n+DEV_CFLAGS += -Wno-unused-parameter\n+endif\n-- 8< --\n"},{"id":"342034","messageId":"20180317151648.GA15772@sigill.intra.peff.net","threadId":"47961","inReplyTo":"D8137CD9-9E11-4FBC-B89B-907C007FC6E9@gmail.com","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-17T15:16:49Z","receivedAt":"2018-03-17T15:16:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:\n\n> >> Why isn't this just turning on DEVELOPER=1 if we know we have a capable\n> >> compiler?\n> > \n> > DEVELOPER=1 is always set even before this patch. It's set and\n> > exported in lib-travisci.sh.\n> \n> I interpreted Peff's comment like this:\n> \n> If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, \n> then we could set your additional flags in the makefile.\n> \n> This way every developer with a new compiler would run these\n> flags locally (if DEVELOPER=1 is set).\n\nActually, I was mostly just confused, and didn't realize that these were\ngoing above and beyond what's in DEVELOPER.\n\nBut that said, now that I understand, I agree completely with your\nsuggestion. :)\n\n-Peff\n"},{"id":"342035","messageId":"20180317160832.GB15772@sigill.intra.peff.net","threadId":"47961","inReplyTo":"20180317145923.GA24689@duynguyen.home","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-17T16:08:32Z","receivedAt":"2018-03-17T16:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 17, 2018 at 03:59:23PM +0100, Duy Nguyen wrote:\n\n> On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:\n> > I interpreted Peff's comment like this:\n> > \n> > If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, \n> > then we could set your additional flags in the makefile.\n> > \n> > This way every developer with a new compiler would run these\n> > flags locally (if DEVELOPER=1 is set).\n> \n> Aha. Something like this? I split developer cflags out to a separate\n> file because I imagine people may start to add gcc7, clang....\n\nYeah, gcc 7 is already standard on debian unstable, so...\n\n> +GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) -dumpversion | cut -f 1 -d .')\n> +\n> +ifeq ($(GCC_VER),6)\n\n...this probably needs to be \"greater than or equal to\".\n\nUnfortunately I think that's hard to do in pure make. But we're already\nrelying on $(shell) here, so we could just move the logic there.\n\nSomething like the patch below, perhaps. It should do the right thing on\nclang and gcc, and I added in an extra clang-only warning I've found\nuseful. Otherwise the list of flags comes from your patch.\n\nThe \"clang4\" limit is just what I happened to have on my system to test\nwith. Some of those flags might work for clang3, and some of the\ngcc6-only flags might work on newer versions of clang.\n\nI was puzzled by -Wno-maybe-uninitialized, though. I think that has\nfound bugs for us before (but also false positives, but we usually\nsquelch those in the code). And it's part of -Wall, not -Wextra, so we\nshouldn't need it as part of this patch, should we?\n\ndiff --git a/Makefile b/Makefile\nindex a1d8775adb..9dfd152a1e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -442,15 +442,6 @@ GIT-VERSION-FILE: FORCE\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n-DEVELOPER_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 LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -1051,7 +1042,7 @@ include config.mak.uname\n -include config.mak\n \n ifdef DEVELOPER\n-CFLAGS += $(DEVELOPER_CFLAGS)\n+include config.mak.dev\n endif\n \n comma := ,\ndiff --git a/config.mak.dev b/config.mak.dev\nnew file mode 100644\nindex 0000000000..59aef342c4\n--- /dev/null\n+++ b/config.mak.dev\n@@ -0,0 +1,28 @@\n+CFLAGS += -Werror\n+CFLAGS += -Wdeclaration-after-statement\n+CFLAGS += -Wno-format-zero-length\n+CFLAGS += -Wold-style-definition\n+CFLAGS += -Woverflow\n+CFLAGS += -Wpointer-arith\n+CFLAGS += -Wstrict-prototypes\n+CFLAGS += -Wunused\n+CFLAGS += -Wvla\n+\n+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n+\n+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wtautological-constant-out-of-range-compare\n+endif\n+\n+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n+CFLAGS += -Wextra\n+CFLAGS += -Wmissing-prototypes\n+CFLAGS += -Wno-empty-body\n+CFLAGS += -Wno-missing-field-initializers\n+CFLAGS += -Wno-sign-compare\n+CFLAGS += -Wno-unused-function\n+CFLAGS += -Wno-unused-parameter\n+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wno-maybe-uninitialized\n+endif\n+endif\ndiff --git a/detect-compiler b/detect-compiler\nnew file mode 100755\nindex 0000000000..043367828c\n--- /dev/null\n+++ b/detect-compiler\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+#\n+# Probe the compiler for vintage, version, etc. This is used for setting\n+# optional make knobs under the DEVELOPER knob.\n+\n+CC=\"$*\"\n+\n+print_flags() {\n+\tfamily=$1\n+\tversion=$($CC -dumpversion | cut -f 1 -d .)\n+\n+\t# Print a feature flag not only for the current version, but also\n+\t# for any prior versions we encompass. This avoids needing to do\n+\t# numeric comparisons in make, which are awkward.\n+\twhile test \"$version\" -gt 0\n+\tdo\n+\t\techo $family$version\n+\t\tversion=$((version - 1))\n+\tdone\n+}\n+\n+case \"$($CC --version 2>&1)\" in\n+gcc*)\n+\tprint_flags gcc\n+\t;;\n+clang*)\n+\tprint_flags clang\n+\t;;\n+*)\n+\t: unknown compiler family\n+\t;;\n+esac\n-- \n2.17.0.rc0.402.ged0b3fd1ee\n\n"},{"id":"342036","messageId":"20180317161208.GA25113@sigill.intra.peff.net","threadId":"47961","inReplyTo":"20180317160832.GB15772@sigill.intra.peff.net","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-17T16:12:08Z","receivedAt":"2018-03-17T16:12:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 17, 2018 at 12:08:32PM -0400, Jeff King wrote:\n\n> +case \"$($CC --version 2>&1)\" in\n> +gcc*)\n> +\tprint_flags gcc\n> +\t;;\n> +clang*)\n> +\tprint_flags clang\n> +\t;;\n> +*)\n> +\t: unknown compiler family\n> +\t;;\n> +esac\n\nBy the way, one annoying thing is that running \"cc --version\" when \"cc\"\nis actually \"gcc\" will print something like:\n\n cc (Debian 7.3.0-11) 7.3.0\n\neven though it supports all of the gcc knobs. This means that:\n\n make DEVELOPER=1\n\nwithout further config won't get these knobs, because we (rightly)\ndefault CC=cc.\n\nProbably this detection could be a bit more clever, like:\n\n cc*Free Software Foundation\")\n\tprint_flags gcc\n\nor something. I don't have any non-gcc/clang compilers to test with, so\nI'm not sure what they even print for \"--version\" (if anything).\n\n-Peff\n"},{"id":"342065","messageId":"xmqqsh8yxnwu.fsf@gitster-ct.c.googlers.com","threadId":"47961","inReplyTo":"20180317160832.GB15772@sigill.intra.peff.net","subject":"Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-17T23:50:09Z","receivedAt":"2018-03-17T23:50:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Unfortunately I think that's hard to do in pure make. But we're already\n> relying on $(shell) here, so we could just move the logic there.\n>\n> Something like the patch below, perhaps. It should do the right thing on\n> clang and gcc, and I added in an extra clang-only warning I've found\n> useful. Otherwise the list of flags comes from your patch.\n\nI see this discussion is going in the right direction, and like the\napproach taken by this \"how about this\" patch.\n\nThanks.\n"},{"id":"342078","messageId":"20180318081834.16081-1-pclouds@gmail.com","threadId":"47961","inReplyTo":"20180317160832.GB15772@sigill.intra.peff.net","subject":"[PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-18T08:18:34Z","receivedAt":"2018-03-18T08:18:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The set of extra warnings we enable when DEVELOPER has to be\nconservative because we can't assume any compiler version the\ndeveloper may use. Detect the compiler version so we know when it's\nsafe to enable -Wextra and maybe more.\n\nHelped-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n -dumpversion does not work correctly for clang. So I use \"-v\" instead\n which seems to produce the same output for both gcc and clang (with a\n minor difference in freebsd where it says \"FreeBSD clang\" instead of\n just \"clang\"). Not sure if it helps your \"cc on debian\" case though\n\n Tested with clang-5.0.1 and gcc-6.4.0 (too lazy to test on freebsd\n clang 3.4.1 but I don't expect surprises there)\n\n I will still need to pull more modern gcc/clang on travis, but that\n can wait until this patch settles.\n\n Makefile | 11 +----------\n config.mak.dev | 28 +++++++++++++++++++++++++++\n detect-compiler | 50 +++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 79 insertions(+), 10 deletions(-)\n create mode 100644 config.mak.dev\n create mode 100755 detect-compiler\n\ndiff --git a/Makefile b/Makefile\nindex a1d8775adb..9dfd152a1e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -442,15 +442,6 @@ GIT-VERSION-FILE: FORCE\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n-DEVELOPER_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 LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -1051,7 +1042,7 @@ include config.mak.uname\n -include config.mak\n \n ifdef DEVELOPER\n-CFLAGS += $(DEVELOPER_CFLAGS)\n+include config.mak.dev\n endif\n \n comma := ,\ndiff --git a/config.mak.dev b/config.mak.dev\nnew file mode 100644\nindex 0000000000..59aef342c4\n--- /dev/null\n+++ b/config.mak.dev\n@@ -0,0 +1,28 @@\n+CFLAGS += -Werror\n+CFLAGS += -Wdeclaration-after-statement\n+CFLAGS += -Wno-format-zero-length\n+CFLAGS += -Wold-style-definition\n+CFLAGS += -Woverflow\n+CFLAGS += -Wpointer-arith\n+CFLAGS += -Wstrict-prototypes\n+CFLAGS += -Wunused\n+CFLAGS += -Wvla\n+\n+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n+\n+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wtautological-constant-out-of-range-compare\n+endif\n+\n+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n+CFLAGS += -Wextra\n+CFLAGS += -Wmissing-prototypes\n+CFLAGS += -Wno-empty-body\n+CFLAGS += -Wno-missing-field-initializers\n+CFLAGS += -Wno-sign-compare\n+CFLAGS += -Wno-unused-function\n+CFLAGS += -Wno-unused-parameter\n+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wno-maybe-uninitialized\n+endif\n+endif\ndiff --git a/detect-compiler b/detect-compiler\nnew file mode 100755\nindex 0000000000..bc2ea39ef5\n--- /dev/null\n+++ b/detect-compiler\n@@ -0,0 +1,50 @@\n+#!/bin/sh\n+#\n+# Probe the compiler for vintage, version, etc. This is used for setting\n+# optional make knobs under the DEVELOPER knob.\n+\n+CC=\"$*\"\n+\n+# we get something like (this is at least true for gcc and clang)\n+#\n+# FreeBSD clang version 3.4.1 (tags/RELEASE...)\n+get_version_line() {\n+\t\"$CC\" -v 2>&1 | grep ' version '\n+}\n+\n+get_family() {\n+\tget_version_line | sed 's/^\\(.*\\) version [0-9][^ ]* .*/\\1/'\n+}\n+\n+get_version() {\n+\tget_version_line | sed 's/^.* version \\([0-9][^ ]*\\) .*/\\1/'\n+}\n+\n+print_flags() {\n+\tfamily=$1\n+\tversion=$(get_version | cut -f 1 -d .)\n+\n+\t# Print a feature flag not only for the current version, but also\n+\t# for any prior versions we encompass. This avoids needing to do\n+\t# numeric comparisons in make, which are awkward.\n+\twhile test \"$version\" -gt 0\n+\tdo\n+\t\techo $family$version\n+\t\tversion=$((version - 1))\n+\tdone\n+}\n+\n+case \"$(get_family)\" in\n+gcc)\n+\tprint_flags gcc\n+\t;;\n+*clang)\n+\tprint_flags clang\n+\t;;\n+\"FreeBSD clang\")\n+\tprint_flags clang\n+\t;;\n+*)\n+\t: unknown compiler family\n+\t;;\n+esac\n-- \n2.17.0.rc0.347.gf9cf61673a\n\n"},{"id":"342084","messageId":"20180318090607.GA26226@flurp.local","threadId":"47961","inReplyTo":"20180318081834.16081-1-pclouds@gmail.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-18T09:06:07Z","receivedAt":"2018-03-18T09:06:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:\n> The set of extra warnings we enable when DEVELOPER has to be\n> conservative because we can't assume any compiler version the\n> developer may use. Detect the compiler version so we know when it's\n> safe to enable -Wextra and maybe more.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> diff --git a/detect-compiler b/detect-compiler\n> --- /dev/null\n> +++ b/detect-compiler\n> @@ -0,0 +1,50 @@\n> +get_version_line() {\n> +\t\"$CC\" -v 2>&1 | grep ' version '\n> +}\n\nOn MacOS, \"cc -v\" output is:\n\n--- >8 ---\nApple LLVM version 9.0.0 (clang-900.0.39.2)\nTarget: x86_64-apple-darwin16.7.0\nThread model: posix\nInstalledDir: ...\n--- >8 ---\n\n> +get_family() {\n> +\tget_version_line | sed 's/^\\(.*\\) version [0-9][^ ]* .*/\\1/'\n> +}\n\nSo, this returns \"Apple LLVM\".\n\n> +case \"$(get_family)\" in\n> +gcc)\n> +\tprint_flags gcc\n> +\t;;\n> +*clang)\n> +\tprint_flags clang\n> +\t;;\n> +\"FreeBSD clang\")\n> +\tprint_flags clang\n> +\t;;\n> +*)\n> +\t: unknown compiler family\n> +\t;;\n> +esac\n\nWhich means you probably want to squash in:\n\n--- >8 ---\ndiff --git a/detect-compiler b/detect-compiler\nindex bc2ea39ef5..3140416b40 100755\n--- a/detect-compiler\n+++ b/detect-compiler\n@@ -41,6 +41,9 @@ gcc)\n *clang)\n \tprint_flags clang\n \t;;\n+\"Apple LLVM\")\n+\tprint_flags clang\n+\t;;\n \"FreeBSD clang\")\n \tprint_flags clang\n \t;;\n--- >8 ---\n"},{"id":"342085","messageId":"CACsJy8BGZV1hHKNzDaFkt=9zfhzv+GR1JUN-fOcQRk_2+tctZA@mail.gmail.com","threadId":"47961","inReplyTo":"20180318090607.GA26226@flurp.local","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-18T09:17:41Z","receivedAt":"2018-03-18T09:20:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine wrote:\n> On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:\n>> The set of extra warnings we enable when DEVELOPER has to be\n>> conservative because we can't assume any compiler version the\n>> developer may use. Detect the compiler version so we know when it's\n>> safe to enable -Wextra and maybe more.\n>>\n>> Helped-by: Jeff King \n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> diff --git a/detect-compiler b/detect-compiler\n>> --- /dev/null\n>> +++ b/detect-compiler\n>> @@ -0,0 +1,50 @@\n>> +get_version_line() {\n>> + \"$CC\" -v 2>&1 | grep ' version '\n>> +}\n>\n> On MacOS, \"cc -v\" output is:\n>\n> --- >8 ---\n> Apple LLVM version 9.0.0 (clang-900.0.39.2)\n> Target: x86_64-apple-darwin16.7.0\n> Thread model: posix\n> InstalledDir: ...\n> --- >8 ---\n\nDoes it still build ok for you with your changes squashed in? I think\nthe check for clang4/gcc6 and setting -Wextra in config.mak.dev may\nbackfire because clang9 probably has a lot more warnings enabled and\nsome of them we don't want and cause compile error...\n-- \nDuy\n"},{"id":"342086","messageId":"20180318092001.GA29208@sigill.intra.peff.net","threadId":"47961","inReplyTo":"CACsJy8BGZV1hHKNzDaFkt=9zfhzv+GR1JUN-fOcQRk_2+tctZA@mail.gmail.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-18T09:20:01Z","receivedAt":"2018-03-18T09:20:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 18, 2018 at 10:17:41AM +0100, Duy Nguyen wrote:\n\n> On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine wrote:\n> > On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:\n> >> The set of extra warnings we enable when DEVELOPER has to be\n> >> conservative because we can't assume any compiler version the\n> >> developer may use. Detect the compiler version so we know when it's\n> >> safe to enable -Wextra and maybe more.\n> >>\n> >> Helped-by: Jeff King \n> >> Signed-off-by: Nguyễn Thái Ngọc Duy \n> >> ---\n> >> diff --git a/detect-compiler b/detect-compiler\n> >> --- /dev/null\n> >> +++ b/detect-compiler\n> >> @@ -0,0 +1,50 @@\n> >> +get_version_line() {\n> >> + \"$CC\" -v 2>&1 | grep ' version '\n> >> +}\n> >\n> > On MacOS, \"cc -v\" output is:\n> >\n> > --- >8 ---\n> > Apple LLVM version 9.0.0 (clang-900.0.39.2)\n> > Target: x86_64-apple-darwin16.7.0\n> > Thread model: posix\n> > InstalledDir: ...\n> > --- >8 ---\n> \n> Does it still build ok for you with your changes squashed in? I think\n> the check for clang4/gcc6 and setting -Wextra in config.mak.dev may\n> backfire because clang9 probably has a lot more warnings enabled and\n> some of them we don't want and cause compile error...\n\nI don't think we can predict that going forward, though. We'd have to\nwait for people to use the new compiler and then send a patch disabling\nwhatever causes a problem (or better yet, fixing the code if\nappropriate).\n\nThis framework can only help us write those patches; I don't think it\ncan save us from having to do them in the first place.\n\n-Peff\n"},{"id":"342087","messageId":"CAPig+cQoU9pYwp==NyQKj_JYZqHqx3LzqSmQ90d3wmMUZ0qA8g@mail.gmail.com","threadId":"47961","inReplyTo":"CACsJy8BGZV1hHKNzDaFkt=9zfhzv+GR1JUN-fOcQRk_2+tctZA@mail.gmail.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-18T09:24:28Z","receivedAt":"2018-03-18T09:24:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 18, 2018 at 5:17 AM, Duy Nguyen wrote:\n> On Sun, Mar 18, 2018 at 10:06 AM, Eric Sunshine wrote:\n>> On MacOS, \"cc -v\" output is:\n>> --- >8 ---\n>> Apple LLVM version 9.0.0 (clang-900.0.39.2)\n>> Target: x86_64-apple-darwin16.7.0\n>> Thread model: posix\n>> InstalledDir: ...\n>> --- >8 ---\n>\n> Does it still build ok for you with your changes squashed in? I think\n> the check for clang4/gcc6 and setting -Wextra in config.mak.dev may\n> backfire because clang9 probably has a lot more warnings enabled and\n> some of them we don't want and cause compile error...\n\nThe project does still compile cleanly for me. (And I used \"make V=1\"\nto verify that the new flags are used.)\n"},{"id":"342088","messageId":"20180318092629.GB29208@sigill.intra.peff.net","threadId":"47961","inReplyTo":"20180318081834.16081-1-pclouds@gmail.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-18T09:26:30Z","receivedAt":"2018-03-18T09:26:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:\n\n> The set of extra warnings we enable when DEVELOPER has to be\n> conservative because we can't assume any compiler version the\n> developer may use. Detect the compiler version so we know when it's\n> safe to enable -Wextra and maybe more.\n\nHrm, I was planning to expand my patch into a series. In particular, in\nmy experiments some of those warnings do not need to be disabled under\n-Wextra (I tested going back to gcc 5 and clang 4.0).\n\nI think it's worth splitting up the boilerplate changes from the\ndecisions on individual warnings, because I suspect we'll need to revise\nthe latter going forward.\n\nI also noticed that all of those options seem to go back at least to\ngcc-5. It makes me wonder how much we even need this version-detecting\nframework. There are one or two \"clang vs gcc\" things I've found, but I\nhaven't found a case where old versions of gcc don't support a\nparticular option.\n\nIOW, could we get away with just adding these to the DEVELOPER knob and\nassuming that people who use it have a reasonably modern gcc or clang\nsetup? That's more or less what the existing knobs do.\n\nBut I didn't follow whether there was any earlier discussion on specific\nproblems.\n\n> -dumpversion does not work correctly for clang. So I use \"-v\" instead\n> which seems to produce the same output for both gcc and clang (with a\n> minor difference in freebsd where it says \"FreeBSD clang\" instead of\n> just \"clang\"). Not sure if it helps your \"cc on debian\" case though\n\nHeh. At first I was confused, as it seems to work for me:\n\n $ clang-4.0 -dumpversion\n 4.2.1\n\nBut then I tried this:\n\n $ clang-5.0 -dumpversion\n 4.2.1\n\nWhoops. It returns the same value up through clang 7.\n\n-Peff\n"},{"id":"342089","messageId":"20180318092825.GC29208@sigill.intra.peff.net","threadId":"47961","inReplyTo":"20180318090607.GA26226@flurp.local","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-18T09:28:26Z","receivedAt":"2018-03-18T09:28:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 18, 2018 at 05:06:07AM -0400, Eric Sunshine wrote:\n\n> On MacOS, \"cc -v\" output is:\n> \n> --- >8 ---\n> Apple LLVM version 9.0.0 (clang-900.0.39.2)\n> Target: x86_64-apple-darwin16.7.0\n> Thread model: posix\n> InstalledDir: ...\n> --- >8 ---\n\nIs that really way ahead of the clang releases (which are at 7.0,\nAFAIK), or do they use a totally different number scheme?\n\nIf the latter, I guess we'd have to treat it separately from clang and\nhave all of the conditionals treat it independently? Yuck.\n\n-Peff\n"},{"id":"342090","messageId":"CAPig+cRQXQ_DowS2Dsc1x3TAGJjnWig7P4eYS4kQ+C2piAdSWA@mail.gmail.com","threadId":"47961","inReplyTo":"20180318092825.GC29208@sigill.intra.peff.net","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-18T09:37:17Z","receivedAt":"2018-03-18T09:37:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 18, 2018 at 5:28 AM, Jeff King wrote:\n> On Sun, Mar 18, 2018 at 05:06:07AM -0400, Eric Sunshine wrote:\n>> On MacOS, \"cc -v\" output is:\n>> --- >8 ---\n>> Apple LLVM version 9.0.0 (clang-900.0.39.2)\n>> Target: x86_64-apple-darwin16.7.0\n>> Thread model: posix\n>> InstalledDir: ...\n>> --- >8 ---\n>\n> Is that really way ahead of the clang releases (which are at 7.0,\n> AFAIK), or do they use a totally different number scheme?\n\nI have no idea, but from past experience with Apple, I'd guess that\nthe version number is an Apple invention.\n\n{...goes and researches a bit...}\n\nIndeed, there doesn't seem to be any correlation between what Apple\nreports and the actual clang version number[1].\n\n[1]: https://stackoverflow.com/questions/33603027/get-apple-clang-version-and-corresponding-upstream-llvm-version\n"},{"id":"342091","messageId":"CACsJy8B-Z7iY0ZHkKtVfLE7RsOAAX4MU17NdkMxwgfO8p3Q3bw@mail.gmail.com","threadId":"47961","inReplyTo":"20180318092629.GB29208@sigill.intra.peff.net","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-18T09:45:00Z","receivedAt":"2018-03-18T09:45:36Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 18, 2018 at 10:26 AM, Jeff King wrote:\n> On Sun, Mar 18, 2018 at 09:18:34AM +0100, Nguyễn Thái Ngọc Duy wrote:\n>\n>> The set of extra warnings we enable when DEVELOPER has to be\n>> conservative because we can't assume any compiler version the\n>> developer may use. Detect the compiler version so we know when it's\n>> safe to enable -Wextra and maybe more.\n>\n> Hrm, I was planning to expand my patch into a series.\n\nI started this so I think people expect me to move it forward. But\nI'll gladly hand it to you. It looks like you have more compilers to\nplay with anyway (I'm on gentoo and installing a new compiler,\nespecially llvm-based, takes ages).\n-- \nDuy\n"},{"id":"342107","messageId":"CACsJy8BOCpHiMxJ0K=_-WPf9b4yh0W0i3m9sAo5-Sk5fY9x1+A@mail.gmail.com","threadId":"47961","inReplyTo":"20180318081834.16081-1-pclouds@gmail.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-18T15:55:25Z","receivedAt":"2018-03-18T15:56:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy wrote:\n> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n> +CFLAGS += -Wextra\n\nAnother thing we can add here is -Og instead of standard -O2 (or -O0\nin my build), which is supported since gcc 4.8. clang seems to support\nit too (mapped to -O1 at least for clang5) but I don't know what\nversion added that flag.\n-- \nDuy\n"},{"id":"342110","messageId":"d30d131e-1cfe-fedb-4a45-7628615a7876@ramsayjones.plus.com","threadId":"47961","inReplyTo":"CACsJy8BOCpHiMxJ0K=_-WPf9b4yh0W0i3m9sAo5-Sk5fY9x1+A@mail.gmail.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-03-18T18:56:51Z","receivedAt":"2018-03-18T18:56:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 18/03/18 15:55, Duy Nguyen wrote:\n> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy wrote:\n>> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n>> +CFLAGS += -Wextra\n> \n> Another thing we can add here is -Og instead of standard -O2 (or -O0\n> in my build), which is supported since gcc 4.8. clang seems to support\n> it too (mapped to -O1 at least for clang5) but I don't know what\n> version added that flag.\n\nI've been doing a lot of compiling recently, using 10 'different'\nversions of clang and gcc ('different' if you count 64-bit and 32-bit\ncompilers with the same version number as different!)\n\nHowever, I can tell you that clang version 3.4 and 3.8.0 don't\nsupport -Og, but clang version 5.0.1 does.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"342198","messageId":"CACsJy8D4QdjncLdX83Xo4g5e-ZbCBqMfN9qaybXx5R3xC9d1gA@mail.gmail.com","threadId":"47961","inReplyTo":"d30d131e-1cfe-fedb-4a45-7628615a7876@ramsayjones.plus.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-19T16:30:11Z","receivedAt":"2018-03-19T16:31:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Mar 18, 2018 at 7:56 PM, Ramsay Jones\n wrote:\n>\n>\n> On 18/03/18 15:55, Duy Nguyen wrote:\n>> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy wrote:\n>>> +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n>>> +CFLAGS += -Wextra\n>>\n>> Another thing we can add here is -Og instead of standard -O2 (or -O0\n>> in my build), which is supported since gcc 4.8. clang seems to support\n>> it too (mapped to -O1 at least for clang5) but I don't know what\n>> version added that flag.\n>\n> I've been doing a lot of compiling recently, using 10 'different'\n> versions of clang and gcc ('different' if you count 64-bit and 32-bit\n> compilers with the same version number as different!)\n>\n> However, I can tell you that clang version 3.4 and 3.8.0 don't\n> support -Og, but clang version 5.0.1 does.\n\nYeah. I checked clang git mirror and this -Og is in 4.x release branch\n(couldn't nail down exact release) so 5.x should be a safe bet.\n-- \nDuy\n"},{"id":"342290","messageId":"20180320053220.GB15813@sigill.intra.peff.net","threadId":"47961","inReplyTo":"CACsJy8BOCpHiMxJ0K=_-WPf9b4yh0W0i3m9sAo5-Sk5fY9x1+A@mail.gmail.com","subject":"Re: [PATCH] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-20T05:32:20Z","receivedAt":"2018-03-20T05:32:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 18, 2018 at 04:55:25PM +0100, Duy Nguyen wrote:\n\n> On Sun, Mar 18, 2018 at 9:18 AM, Nguyễn Thái Ngọc Duy wrote:\n> > +ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n> > +CFLAGS += -Wextra\n> \n> Another thing we can add here is -Og instead of standard -O2 (or -O0\n> in my build), which is supported since gcc 4.8. clang seems to support\n> it too (mapped to -O1 at least for clang5) but I don't know what\n> version added that flag.\n\nI don't know, that feels kind of weird to me. I thought DEVELOPER was\nsupposed to mean \"ratchet up the linting, I want to know if I've broken\nsomething\".\n\nBut tweaking -O is about \"I am in an edit-compile-debug cycle\".\nSometimes you are and sometimes you aren't, but you'd presumably want to\nhave extra warnings regardless (especially because some warnings only\ntrigger under particular optimization settings).\n\nPersonally, I default to -O0, but then crank up to -O2 for performance\ntesting or for my daily-driver builds. But I _always_ have as many\nwarnings enabled as possible[1].\n\n-Peff\n\n[1] I do have some pretty horrific magic to turn on -Werror when I'm\n visiting a \"historical\" commit, such as during a bisect.\n"},{"id":"342819","messageId":"20180324125348.6614-1-pclouds@gmail.com","threadId":"47961","inReplyTo":"20180318081834.16081-1-pclouds@gmail.com","subject":"[PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-24T12:53:48Z","receivedAt":"2018-03-24T12:54:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The set of extra warnings we enable when DEVELOPER has to be\nconservative because we can't assume any compiler version the\ndeveloper may use. Detect the compiler version so we know when it's\nsafe to enable -Wextra and maybe more.\n\nThese warning settings are mostly from my custom config.mak a long\ntime ago when I tried to enable as many warnings as possible that can\nstill build without showing warnings. Some of them those warnings are\nprobably worth fixing instead of just suppressing in future.\n\nHelped-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 improves a bit over v1:\n\n - apple clang support (though I suspect we may want to make it a\n separate compiler family since apple clang has different\n versioning, but I can't and won't work on that front)\n\n - support CC='ccache gcc' (yes it breaks now if you have spaces in\n your cc path)\n\n - allow to skip detect-compiler by setting COMPILER_FEATURES in\n config.mak\n\n I notice Ramsay is working on clean -Wmaybe-uninitialized, if his\n series is merged first I'll stop disabling it here.\n\n Interdiff\n\n diff --git a/Makefile b/Makefile\n index 9dfd152a1e..04b2a39bab 100644\n --- a/Makefile\n +++ b/Makefile\n @@ -434,6 +434,10 @@ all::\n #\n # When cross-compiling, define HOST_CPU as the canonical name of the CPU on\n # which the built Git will run (for instance \"x86_64\").\n +#\n +# Define DEVELOPER to enable more compiler warnings. Compiler version\n +# and faimily are auto detected, but could be overridden by defining\n +# COMPILER_FEATURES (see config.mak.dev)\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n diff --git a/config.mak.dev b/config.mak.dev\n index 59aef342c4..d8beaf9347 100644\n --- a/config.mak.dev\n +++ b/config.mak.dev\n @@ -8,7 +8,9 @@ CFLAGS += -Wstrict-prototypes\n CFLAGS += -Wunused\n CFLAGS += -Wvla\n \n +ifndef COMPILER_FEATURES\n COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n +endif\n \n ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n CFLAGS += -Wtautological-constant-out-of-range-compare\n diff --git a/detect-compiler b/detect-compiler\n index bc2ea39ef5..70b754481c 100755\n --- a/detect-compiler\n +++ b/detect-compiler\n @@ -9,7 +9,7 @@ CC=\"$*\"\n #\n # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n get_version_line() {\n -\t\"$CC\" -v 2>&1 | grep ' version '\n +\t$CC -v 2>&1 | grep ' version '\n }\n \n get_family() {\n @@ -38,12 +38,15 @@ case \"$(get_family)\" in\n gcc)\n \tprint_flags gcc\n \t;;\n -*clang)\n +clang)\n \tprint_flags clang\n \t;;\n \"FreeBSD clang\")\n \tprint_flags clang\n \t;;\n +\"Apple LLVM\")\n +\tprint_flags clang\n +\t;;\n *)\n \t: unknown compiler family\n \t;;\n\n Makefile | 15 +++++---------\n config.mak.dev | 30 ++++++++++++++++++++++++++++\n detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 88 insertions(+), 10 deletions(-)\n create mode 100644 config.mak.dev\n create mode 100755 detect-compiler\n\ndiff --git a/Makefile b/Makefile\nindex a1d8775adb..04b2a39bab 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -434,6 +434,10 @@ all::\n #\n # When cross-compiling, define HOST_CPU as the canonical name of the CPU on\n # which the built Git will run (for instance \"x86_64\").\n+#\n+# Define DEVELOPER to enable more compiler warnings. Compiler version\n+# and faimily are auto detected, but could be overridden by defining\n+# COMPILER_FEATURES (see config.mak.dev)\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -442,15 +446,6 @@ GIT-VERSION-FILE: FORCE\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n-DEVELOPER_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 LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -1051,7 +1046,7 @@ include config.mak.uname\n -include config.mak\n \n ifdef DEVELOPER\n-CFLAGS += $(DEVELOPER_CFLAGS)\n+include config.mak.dev\n endif\n \n comma := ,\ndiff --git a/config.mak.dev b/config.mak.dev\nnew file mode 100644\nindex 0000000000..d8beaf9347\n--- /dev/null\n+++ b/config.mak.dev\n@@ -0,0 +1,30 @@\n+CFLAGS += -Werror\n+CFLAGS += -Wdeclaration-after-statement\n+CFLAGS += -Wno-format-zero-length\n+CFLAGS += -Wold-style-definition\n+CFLAGS += -Woverflow\n+CFLAGS += -Wpointer-arith\n+CFLAGS += -Wstrict-prototypes\n+CFLAGS += -Wunused\n+CFLAGS += -Wvla\n+\n+ifndef COMPILER_FEATURES\n+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n+endif\n+\n+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wtautological-constant-out-of-range-compare\n+endif\n+\n+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n+CFLAGS += -Wextra\n+CFLAGS += -Wmissing-prototypes\n+CFLAGS += -Wno-empty-body\n+CFLAGS += -Wno-missing-field-initializers\n+CFLAGS += -Wno-sign-compare\n+CFLAGS += -Wno-unused-function\n+CFLAGS += -Wno-unused-parameter\n+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wno-maybe-uninitialized\n+endif\n+endif\ndiff --git a/detect-compiler b/detect-compiler\nnew file mode 100755\nindex 0000000000..70b754481c\n--- /dev/null\n+++ b/detect-compiler\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+#\n+# Probe the compiler for vintage, version, etc. This is used for setting\n+# optional make knobs under the DEVELOPER knob.\n+\n+CC=\"$*\"\n+\n+# we get something like (this is at least true for gcc and clang)\n+#\n+# FreeBSD clang version 3.4.1 (tags/RELEASE...)\n+get_version_line() {\n+\t$CC -v 2>&1 | grep ' version '\n+}\n+\n+get_family() {\n+\tget_version_line | sed 's/^\\(.*\\) version [0-9][^ ]* .*/\\1/'\n+}\n+\n+get_version() {\n+\tget_version_line | sed 's/^.* version \\([0-9][^ ]*\\) .*/\\1/'\n+}\n+\n+print_flags() {\n+\tfamily=$1\n+\tversion=$(get_version | cut -f 1 -d .)\n+\n+\t# Print a feature flag not only for the current version, but also\n+\t# for any prior versions we encompass. This avoids needing to do\n+\t# numeric comparisons in make, which are awkward.\n+\twhile test \"$version\" -gt 0\n+\tdo\n+\t\techo $family$version\n+\t\tversion=$((version - 1))\n+\tdone\n+}\n+\n+case \"$(get_family)\" in\n+gcc)\n+\tprint_flags gcc\n+\t;;\n+clang)\n+\tprint_flags clang\n+\t;;\n+\"FreeBSD clang\")\n+\tprint_flags clang\n+\t;;\n+\"Apple LLVM\")\n+\tprint_flags clang\n+\t;;\n+*)\n+\t: unknown compiler family\n+\t;;\n+esac\n-- \n2.17.0.rc0.348.gd5a49e0b6f\n\n"},{"id":"342855","messageId":"CAPig+cQAufB=DDgkSwz+EDkkTKEx32KJBGPdeDxuzZTdoM2Nrg@mail.gmail.com","threadId":"47961","inReplyTo":"20180324125348.6614-1-pclouds@gmail.com","subject":"Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-25T00:40:12Z","receivedAt":"2018-03-25T00:40:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 24, 2018 at 8:53 AM, Nguyễn Thái Ngọc Duy wrote:\n> The set of extra warnings we enable when DEVELOPER has to be\n> conservative because we can't assume any compiler version the\n> developer may use. Detect the compiler version so we know when it's\n> safe to enable -Wextra and maybe more.\n>\n> These warning settings are mostly from my custom config.mak a long\n> time ago when I tried to enable as many warnings as possible that can\n> still build without showing warnings. Some of them those warnings are\n\ns/them those/those/\n\n> probably worth fixing instead of just suppressing in future.\n>\n> Helped-by: Jeff King \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"343077","messageId":"xmqqlgeebiml.fsf@gitster-ct.c.googlers.com","threadId":"47961","inReplyTo":"20180324125348.6614-1-pclouds@gmail.com","subject":"Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-26T22:02:26Z","receivedAt":"2018-03-26T22:02:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> The set of extra warnings we enable when DEVELOPER has to be\n> conservative because we can't assume any compiler version the\n> developer may use. Detect the compiler version so we know when it's\n> safe to enable -Wextra and maybe more.\n\nThis is a good idea in general, but we are not quite ready without\nsome fixups. \n\nHere is a quick summary (not exhaustive) from my trial merge to 'pu'\n(which will be reverted before today's integration is pushed out).\n\n - json-writer.c triggers -Werror=old-style-decl\n\n - t/helper/test-json-writer.c triggers Werror=missing-prototypes\n quite a few times.\n\n - connect.c -Werror=implicit-fallthough around die_initial_contact().\n\n"},{"id":"343133","messageId":"CACsJy8B_KPGroNVWskeTXu9NuYoSUAoSvYpfapda1_0A1PUwPg@mail.gmail.com","threadId":"47961","inReplyTo":"xmqqlgeebiml.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-27T15:03:14Z","receivedAt":"2018-03-27T15:03:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> The set of extra warnings we enable when DEVELOPER has to be\n>> conservative because we can't assume any compiler version the\n>> developer may use. Detect the compiler version so we know when it's\n>> safe to enable -Wextra and maybe more.\n>\n> This is a good idea in general, but we are not quite ready without\n> some fixups.\n>\n> Here is a quick summary (not exhaustive) from my trial merge to 'pu'\n> (which will be reverted before today's integration is pushed out).\n>\n> - json-writer.c triggers -Werror=old-style-decl\n>\n> - t/helper/test-json-writer.c triggers Werror=missing-prototypes\n> quite a few times.\n\nI expected these (and it was a good reason to push this patch\nforward). I think people also reported style problems with this\nseries.\n\n>\n> - connect.c -Werror=implicit-fallthough around die_initial_contact().\n>\n\nThis I did not expect. But I just looked again and I had this option\nexplicitly turned off in config.mak! gcc-6.4 and gcc-4.9 do not\ncomplain about this. gcc-7.3 does. What's your compiler/version?\n\n\n-- \nDuy\n"},{"id":"343149","messageId":"xmqqh8p1a2b8.fsf@gitster-ct.c.googlers.com","threadId":"47961","inReplyTo":"CACsJy8B_KPGroNVWskeTXu9NuYoSUAoSvYpfapda1_0A1PUwPg@mail.gmail.com","subject":"Re: [PATCH v2] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-27T16:52:27Z","receivedAt":"2018-03-27T16:52:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n On Tue, Mar 27, 2018 at 12:02 AM, Junio C Hamano wrote:\n>>\n>> - connect.c -Werror=implicit-fallthough around die_initial_contact().\n>\n> This I did not expect. But I just looked again and I had this option\n> explicitly turned off in config.mak! gcc-6.4 and gcc-4.9 do not\n> complain about this. gcc-7.3 does. What's your compiler/version?\n\ngcc (Debian 7.3.0-5) 7.3.0\n"},{"id":"343330","messageId":"20180329150322.10722-4-pclouds@gmail.com","threadId":"47961","inReplyTo":"20180329150322.10722-1-pclouds@gmail.com","subject":"[PATCH v3 3/3] Makefile: add EAGER_DEVELOPER mode","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T15:03:22Z","receivedAt":"2018-03-29T15:03:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This mode is for developers who want to keep the code base\nclean. There are warning classes that are currently suppressed because\nwe have too many of them in the code base. This mode will stop the\nsuppression, let the developer see and decide how to fix them.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Makefile | 7 ++++++-\n config.mak.dev | 4 ++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex e6680a8977..e4f04ce1cb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -434,7 +434,9 @@ all::\n #\n # Define DEVELOPER to enable more compiler warnings. Compiler version\n # and faimily are auto detected, but could be overridden by defining\n-# COMPILER_FEATURES (see config.mak.dev)\n+# COMPILER_FEATURES (see config.mak.dev).\n+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning\n+# class is suppressed anymore.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -1041,6 +1043,9 @@ include config.mak.uname\n -include config.mak.autogen\n -include config.mak\n \n+ifdef EAGER_DEVELOPER\n+DEVELOPER = Yes\n+endif\n ifdef DEVELOPER\n include config.mak.dev\n endif\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 716a14ecc7..13883410b3 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -1,4 +1,6 @@\n+ifndef EAGER_DEVELOPER\n CFLAGS += -Werror\n+endif\n CFLAGS += -Wdeclaration-after-statement\n CFLAGS += -Wno-format-zero-length\n CFLAGS += -Wold-style-definition\n@@ -21,6 +23,7 @@ CFLAGS += -Wextra\n # if a function is public, there should be a prototype and the right\n # header file should be included. If not, it should be static.\n CFLAGS += -Wmissing-prototypes\n+ifndef EAGER_DEVELOPER\n # These are disabled because we have these all over the place.\n CFLAGS += -Wno-empty-body\n CFLAGS += -Wno-missing-field-initializers\n@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare\n CFLAGS += -Wno-unused-function\n CFLAGS += -Wno-unused-parameter\n endif\n+endif\n \n # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c\n # not worth fixing since newer compilers correctly stop complaining\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343331","messageId":"20180329150322.10722-3-pclouds@gmail.com","threadId":"47961","inReplyTo":"20180329150322.10722-1-pclouds@gmail.com","subject":"[PATCH v3 2/3] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T15:03:21Z","receivedAt":"2018-03-29T15:03:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The set of extra warnings we enable when DEVELOPER has to be\nconservative because we can't assume any compiler version the\ndeveloper may use. Detect the compiler version so we know when it's\nsafe to enable -Wextra and maybe more.\n\nThese warning settings are mostly from my custom config.mak a long\ntime ago when I tried to enable as many warnings as possible that can\nstill build without showing warnings. Some of those warnings are\nprobably worth fixing instead of just suppressing in future.\n\nHelped-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Makefile | 15 +++++---------\n config.mak.dev | 38 +++++++++++++++++++++++++++++++++++\n detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 96 insertions(+), 10 deletions(-)\n create mode 100644 config.mak.dev\n create mode 100755 detect-compiler\n\ndiff --git a/Makefile b/Makefile\nindex 7f40f76739..e6680a8977 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -431,6 +431,10 @@ all::\n #\n # When cross-compiling, define HOST_CPU as the canonical name of the CPU on\n # which the built Git will run (for instance \"x86_64\").\n+#\n+# Define DEVELOPER to enable more compiler warnings. Compiler version\n+# and faimily are auto detected, but could be overridden by defining\n+# COMPILER_FEATURES (see config.mak.dev)\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -439,15 +443,6 @@ GIT-VERSION-FILE: FORCE\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n-DEVELOPER_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 LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -1047,7 +1042,7 @@ include config.mak.uname\n -include config.mak\n \n ifdef DEVELOPER\n-CFLAGS += $(DEVELOPER_CFLAGS)\n+include config.mak.dev\n endif\n \n comma := ,\ndiff --git a/config.mak.dev b/config.mak.dev\nnew file mode 100644\nindex 0000000000..716a14ecc7\n--- /dev/null\n+++ b/config.mak.dev\n@@ -0,0 +1,38 @@\n+CFLAGS += -Werror\n+CFLAGS += -Wdeclaration-after-statement\n+CFLAGS += -Wno-format-zero-length\n+CFLAGS += -Wold-style-definition\n+CFLAGS += -Woverflow\n+CFLAGS += -Wpointer-arith\n+CFLAGS += -Wstrict-prototypes\n+CFLAGS += -Wunused\n+CFLAGS += -Wvla\n+\n+ifndef COMPILER_FEATURES\n+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n+endif\n+\n+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wtautological-constant-out-of-range-compare\n+endif\n+\n+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n+CFLAGS += -Wextra\n+# if a function is public, there should be a prototype and the right\n+# header file should be included. If not, it should be static.\n+CFLAGS += -Wmissing-prototypes\n+# These are disabled because we have these all over the place.\n+CFLAGS += -Wno-empty-body\n+CFLAGS += -Wno-missing-field-initializers\n+CFLAGS += -Wno-sign-compare\n+CFLAGS += -Wno-unused-function\n+CFLAGS += -Wno-unused-parameter\n+endif\n+\n+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c\n+# not worth fixing since newer compilers correctly stop complaining\n+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)\n+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wno-uninitialized\n+endif\n+endif\ndiff --git a/detect-compiler b/detect-compiler\nnew file mode 100755\nindex 0000000000..70b754481c\n--- /dev/null\n+++ b/detect-compiler\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+#\n+# Probe the compiler for vintage, version, etc. This is used for setting\n+# optional make knobs under the DEVELOPER knob.\n+\n+CC=\"$*\"\n+\n+# we get something like (this is at least true for gcc and clang)\n+#\n+# FreeBSD clang version 3.4.1 (tags/RELEASE...)\n+get_version_line() {\n+\t$CC -v 2>&1 | grep ' version '\n+}\n+\n+get_family() {\n+\tget_version_line | sed 's/^\\(.*\\) version [0-9][^ ]* .*/\\1/'\n+}\n+\n+get_version() {\n+\tget_version_line | sed 's/^.* version \\([0-9][^ ]*\\) .*/\\1/'\n+}\n+\n+print_flags() {\n+\tfamily=$1\n+\tversion=$(get_version | cut -f 1 -d .)\n+\n+\t# Print a feature flag not only for the current version, but also\n+\t# for any prior versions we encompass. This avoids needing to do\n+\t# numeric comparisons in make, which are awkward.\n+\twhile test \"$version\" -gt 0\n+\tdo\n+\t\techo $family$version\n+\t\tversion=$((version - 1))\n+\tdone\n+}\n+\n+case \"$(get_family)\" in\n+gcc)\n+\tprint_flags gcc\n+\t;;\n+clang)\n+\tprint_flags clang\n+\t;;\n+\"FreeBSD clang\")\n+\tprint_flags clang\n+\t;;\n+\"Apple LLVM\")\n+\tprint_flags clang\n+\t;;\n+*)\n+\t: unknown compiler family\n+\t;;\n+esac\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343332","messageId":"20180329150322.10722-1-pclouds@gmail.com","threadId":"47961","inReplyTo":"20180324125348.6614-1-pclouds@gmail.com","subject":"[PATCH v3 0/3] Enable more compiler warnings for devs","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T15:03:19Z","receivedAt":"2018-03-29T15:03:56Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v3 fixes the fallthru warnings in connect.c, and with json-writer\nseries rerolled, 'pu' should build cleanly now.\n-Wno-maybe-uninitialized is removed, thanks to Ramsay.\n\nv3 also adds an experimental patch that adds EAGER_DEVELOPER=1. These\ndevelopers will have all warnings enabled (nothing is suppressed) but\nthey are not fatal. They could go through them and perhaps clean up\nthe code base a bit more.\n\nInterdiff\n\ndiff --git a/Makefile b/Makefile\nindex e6680a8977..e4f04ce1cb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -434,7 +434,9 @@ all::\n #\n # Define DEVELOPER to enable more compiler warnings. Compiler version\n # and faimily are auto detected, but could be overridden by defining\n-# COMPILER_FEATURES (see config.mak.dev)\n+# COMPILER_FEATURES (see config.mak.dev).\n+# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning\n+# class is suppressed anymore.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -1041,6 +1043,9 @@ include config.mak.uname\n -include config.mak.autogen\n -include config.mak\n \n+ifdef EAGER_DEVELOPER\n+DEVELOPER = Yes\n+endif\n ifdef DEVELOPER\n include config.mak.dev\n endif\ndiff --git a/config.mak.dev b/config.mak.dev\nindex d8beaf9347..13883410b3 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -1,4 +1,6 @@\n+ifndef EAGER_DEVELOPER\n CFLAGS += -Werror\n+endif\n CFLAGS += -Wdeclaration-after-statement\n CFLAGS += -Wno-format-zero-length\n CFLAGS += -Wold-style-definition\n@@ -18,13 +20,23 @@ endif\n \n ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n CFLAGS += -Wextra\n+# if a function is public, there should be a prototype and the right\n+# header file should be included. If not, it should be static.\n CFLAGS += -Wmissing-prototypes\n+ifndef EAGER_DEVELOPER\n+# These are disabled because we have these all over the place.\n CFLAGS += -Wno-empty-body\n CFLAGS += -Wno-missing-field-initializers\n CFLAGS += -Wno-sign-compare\n CFLAGS += -Wno-unused-function\n CFLAGS += -Wno-unused-parameter\n-ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)\n-CFLAGS += -Wno-maybe-uninitialized\n+endif\n+endif\n+\n+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c\n+# not worth fixing since newer compilers correctly stop complaining\n+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)\n+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wno-uninitialized\n endif\n endif\ndiff --git a/connect.c b/connect.c\nindex c3a014c5ba..49eca46462 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)\n \treturn check_ref(ref->name, flags);\n }\n \n-static void die_initial_contact(int unexpected)\n+static NORETURN void die_initial_contact(int unexpected)\n {\n \tif (unexpected)\n \t\tdie(_(\"The remote end hung up upon initial contact\"));\n\n\nNguyễn Thái Ngọc Duy (3):\n connect.c: mark die_initial_contact() NORETURN\n Makefile: detect compiler and enable more warnings in DEVELOPER=1\n Makefile: add EAGER_DEVELOPER mode\n\n Makefile | 20 +++++++++----------\n config.mak.dev | 42 +++++++++++++++++++++++++++++++++++++++\n connect.c | 2 +-\n detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 106 insertions(+), 11 deletions(-)\n create mode 100644 config.mak.dev\n create mode 100755 detect-compiler\n\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343333","messageId":"20180329150322.10722-2-pclouds@gmail.com","threadId":"47961","inReplyTo":"20180329150322.10722-1-pclouds@gmail.com","subject":"[PATCH v3 1/3] connect.c: mark die_initial_contact() NORETURN","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-29T15:03:20Z","receivedAt":"2018-03-29T15:03:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a series running in parallel with this one that adds code\nlike this\n\n switch (...) {\n case ...:\n die_initial_contact();\n case ...:\n\nThere is nothing wrong with this. There is no actual falling\nthrough. But since gcc is not that smart and gcc 7.x introduces\n-Wimplicit-fallthrough, it raises a false alarm in this case.\n\nThis class of warnings may be useful elsewhere, so instead of\nsuppressing the whole class, let's try to fix just this code. gcc is\nsmart enough to realize that no execution can continue after a\nNORETURN function call and no longer raises the warning.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n connect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex c3a014c5ba..49eca46462 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)\n \treturn check_ref(ref->name, flags);\n }\n \n-static void die_initial_contact(int unexpected)\n+static NORETURN void die_initial_contact(int unexpected)\n {\n \tif (unexpected)\n \t\tdie(_(\"The remote end hung up upon initial contact\"));\n-- \n2.17.0.rc1.439.gca064e2955\n\n"},{"id":"343537","messageId":"20180331164009.2264-1-avarab@gmail.com","threadId":"47961","inReplyTo":"20180329150322.10722-1-pclouds@gmail.com","subject":"[PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-03-31T16:40:09Z","receivedAt":"2018-03-31T16:40:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag\nwhich (approximately) enables -Wextra so that any combination of them\nand -Werror can be set.\n\nI've long wanted to use DEVELOPER=1 in my production builds, but on\nsome old systems I still get warnings, and thus the build would\nfail. However if the build/tests fail for some other reason, it would\nstill be useful to scroll up and see what the relevant code is warning\nabout.\n\nThis change allows for that. Now setting DEVELOPER will set -Werror as\nbefore, but if DEVELOPER_NONFATAL is set you'll get the same warnings,\nbut without -Werror.\n\nI've renamed the newly added EAGER_DEVELOPER flag to\nDEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,\nand it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than\ninventing some new name of our own (VERY_EAGER_DEVELOPER?).\n\nThe DEVELOPER_EXTRA flag implicitly means DEVELOPER_NONFATAL, but just\nso that this change doesn't introduce yet another arbitrary limitation\nit's possible to combine it with DEVELOPER_FATAL, which will turn on\n-Werror for it as well.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nI really like where this is going, but as noted I think this on top\nwould be even better.\n\n Makefile | 17 +++++++++++------\n config.mak.dev | 10 ++++++++--\n 2 files changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex e4f04ce1cb..641461a569 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -432,11 +432,16 @@ all::\n # When cross-compiling, define HOST_CPU as the canonical name of the CPU on\n # which the built Git will run (for instance \"x86_64\").\n #\n-# Define DEVELOPER to enable more compiler warnings. Compiler version\n-# and faimily are auto detected, but could be overridden by defining\n-# COMPILER_FEATURES (see config.mak.dev).\n-# Define EAGER_DEVELOPER keeps compiler warnings non-fatal, but no warning\n-# class is suppressed anymore.\n+# Define DEVELOPER to enable more compiler warnings. We'll also enable\n+# -Werror unless DEVELOPER_NONFATAL is defined. To enable even more\n+# pedantic warnings that'll flag some potential existing issues in the\n+# codebase turn on DEVELOPER_EXTRA, which implicitly sets DEVELOPER as\n+# well, This is -Wextra with a whitelist of disabled warnings. Unless\n+# DEVELOPER_NONFATAL is set DEVELOPER_EXTRA will turn it on\n+# implicitly, so if you for some reason want both DEVELOPER and\n+# DEVELOPER_EXTRA with fatal warnings, you need to set\n+# DEVELOPER_FATAL=1 to force -Werror. See config.mak.dev for how this\n+# all works.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -1043,7 +1048,7 @@ include config.mak.uname\n -include config.mak.autogen\n -include config.mak\n \n-ifdef EAGER_DEVELOPER\n+ifdef DEVELOPER_EXTRA\n DEVELOPER = Yes\n endif\n ifdef DEVELOPER\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 13883410b3..40f3365729 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -1,6 +1,12 @@\n-ifndef EAGER_DEVELOPER\n+ifndef DEVELOPER_NONFATAL\n+ifndef DEVELOPER_EXTRA\n CFLAGS += -Werror\n endif\n+endif\n+ifdef DEVELOPER_FATAL\n+CFLAGS += -Werror\n+endif\n+\n CFLAGS += -Wdeclaration-after-statement\n CFLAGS += -Wno-format-zero-length\n CFLAGS += -Wold-style-definition\n@@ -23,7 +29,7 @@ CFLAGS += -Wextra\n # if a function is public, there should be a prototype and the right\n # header file should be included. If not, it should be static.\n CFLAGS += -Wmissing-prototypes\n-ifndef EAGER_DEVELOPER\n+ifndef DEVELOPER_EXTRA\n # These are disabled because we have these all over the place.\n CFLAGS += -Wno-empty-body\n CFLAGS += -Wno-missing-field-initializers\n-- \n2.17.0.rc1.321.gba9d0f2565\n\n"},{"id":"343541","messageId":"CACsJy8CyB0igY365NMkswSgAi9_rf+XBOMQyJ7XW6iQxQiCEyQ@mail.gmail.com","threadId":"47961","inReplyTo":"20180331164009.2264-1-avarab@gmail.com","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-31T18:36:39Z","receivedAt":"2018-03-31T18:37:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag\n> which (approximately) enables -Wextra so that any combination of them\n> and -Werror can be set.\n>\n> I've long wanted to use DEVELOPER=1 in my production builds, but on\n> some old systems I still get warnings, and thus the build would\n> fail. However if the build/tests fail for some other reason, it would\n> still be useful to scroll up and see what the relevant code is warning\n> about.\n>\n> This change allows for that. Now setting DEVELOPER will set -Werror as\n> before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,\n> but without -Werror.\n>\n> I've renamed the newly added EAGER_DEVELOPER flag to\n> DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,\n> and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than\n> inventing some new name of our own (VERY_EAGER_DEVELOPER?).\n\nBefore we go with zillions of *DEVELOPER* maybe we can have something\nlike DEVOPTS where you can give multiple keywords to a single variable\nto influence config.mak.dev. This is similar to COMPILER_FEATURES we\nalready have in there, but now it's driven by the dev instead of the\ncompiler. So you can have keywords like \"gentle\" (no -Werror) \"extra\"\n(-Wextra with no suppression) and something else.\n-- \nDuy\n"},{"id":"343667","messageId":"87woxove8d.fsf@evledraar.gmail.com","threadId":"47961","inReplyTo":"CACsJy8CyB0igY365NMkswSgAi9_rf+XBOMQyJ7XW6iQxQiCEyQ@mail.gmail.com","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-03T09:19:46Z","receivedAt":"2018-04-03T09:19:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Mar 31 2018, Duy Nguyen wrote:\n\n> On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag\n>> which (approximately) enables -Wextra so that any combination of them\n>> and -Werror can be set.\n>>\n>> I've long wanted to use DEVELOPER=1 in my production builds, but on\n>> some old systems I still get warnings, and thus the build would\n>> fail. However if the build/tests fail for some other reason, it would\n>> still be useful to scroll up and see what the relevant code is warning\n>> about.\n>>\n>> This change allows for that. Now setting DEVELOPER will set -Werror as\n>> before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,\n>> but without -Werror.\n>>\n>> I've renamed the newly added EAGER_DEVELOPER flag to\n>> DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,\n>> and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than\n>> inventing some new name of our own (VERY_EAGER_DEVELOPER?).\n>\n> Before we go with zillions of *DEVELOPER* maybe we can have something\n> like DEVOPTS where you can give multiple keywords to a single variable\n> to influence config.mak.dev. This is similar to COMPILER_FEATURES we\n> already have in there, but now it's driven by the dev instead of the\n> compiler. So you can have keywords like \"gentle\" (no -Werror) \"extra\"\n> (-Wextra with no suppression) and something else.\n\nWe could do that, but I don't think it's that bad. This patch is one\nextra option on top of yours, and it's not going to result in some\ncombinatorial explosion of options, i.e. if we add DEVELOPER_PEDANTIC\nwe'll just add one extra flag.\n\nBut sure, we could make this some string we'd need to parse out similar\nto COMPILER_FEATURES, it just seems more complex to me for this task.\n"},{"id":"343699","messageId":"20180403151700.GA24602@duynguyen.home","threadId":"47961","inReplyTo":"87woxove8d.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-04-03T15:17:00Z","receivedAt":"2018-04-03T15:17:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Apr 03, 2018 at 11:19:46AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sat, Mar 31 2018, Duy Nguyen wrote:\n> \n> > On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason\n> > wrote:\n> >> Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag\n> >> which (approximately) enables -Wextra so that any combination of them\n> >> and -Werror can be set.\n> >>\n> >> I've long wanted to use DEVELOPER=1 in my production builds, but on\n> >> some old systems I still get warnings, and thus the build would\n> >> fail. However if the build/tests fail for some other reason, it would\n> >> still be useful to scroll up and see what the relevant code is warning\n> >> about.\n> >>\n> >> This change allows for that. Now setting DEVELOPER will set -Werror as\n> >> before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,\n> >> but without -Werror.\n> >>\n> >> I've renamed the newly added EAGER_DEVELOPER flag to\n> >> DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,\n> >> and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than\n> >> inventing some new name of our own (VERY_EAGER_DEVELOPER?).\n> >\n> > Before we go with zillions of *DEVELOPER* maybe we can have something\n> > like DEVOPTS where you can give multiple keywords to a single variable\n> > to influence config.mak.dev. This is similar to COMPILER_FEATURES we\n> > already have in there, but now it's driven by the dev instead of the\n> > compiler. So you can have keywords like \"gentle\" (no -Werror) \"extra\"\n> > (-Wextra with no suppression) and something else.\n> \n> We could do that, but I don't think it's that bad. This patch is one\n> extra option on top of yours,\n\nAnd that eager this was marked experimental because I was not sure if\nit was the right way :)\n\n> and it's not going to result in some combinatorial explosion of\n> options, i.e. if we add DEVELOPER_PEDANTIC we'll just add one extra\n> flag.\n> \n> But sure, we could make this some string we'd need to parse out similar\n> to COMPILER_FEATURES, it just seems more complex to me for this task.\n\nIt's not that complex. With the EAGER_DEVELOPER patch removed, we can\nhave something like this where eager devs just need to put\n\n DEVOPTS = gentle no-suppression\n\nand you put\n\n DEVOPTS = gentle\n\n(bad naming, I didn't spend time thinking about names)\n\n-- 8< --\ndiff --git a/Makefile b/Makefile\nindex e6680a8977..7b4e038e1d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -435,6 +435,11 @@ all::\n # Define DEVELOPER to enable more compiler warnings. Compiler version\n # and faimily are auto detected, but could be overridden by defining\n # COMPILER_FEATURES (see config.mak.dev)\n+#\n+# When DEVELOPER is set, DEVOPTS can be used to control compiler options.\n+# This variable contains keywords separated by whitespace. Two keywords\n+# are recognized: \"gentle\" removes -Werror and \"no-suppression\"\n+# removes all \"-Wno-\" options.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 716a14ecc7..1d7aba6a6a 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -1,4 +1,6 @@\n+ifeq ($(filter gentle,$(DEVOPTS)),)\n CFLAGS += -Werror\n+endif\n CFLAGS += -Wdeclaration-after-statement\n CFLAGS += -Wno-format-zero-length\n CFLAGS += -Wold-style-definition\n@@ -21,6 +23,7 @@ CFLAGS += -Wextra\n # if a function is public, there should be a prototype and the right\n # header file should be included. If not, it should be static.\n CFLAGS += -Wmissing-prototypes\n+ifeq ($(filter no-suppression,$(DEVOPTS)),)\n # These are disabled because we have these all over the place.\n CFLAGS += -Wno-empty-body\n CFLAGS += -Wno-missing-field-initializers\n@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare\n CFLAGS += -Wno-unused-function\n CFLAGS += -Wno-unused-parameter\n endif\n+endif\n \n # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c\n # not worth fixing since newer compilers correctly stop complaining\n-- 8< --\n"},{"id":"344015","messageId":"20180406214236.GF7870@sigill.intra.peff.net","threadId":"47961","inReplyTo":"20180403151700.GA24602@duynguyen.home","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-06T21:42:36Z","receivedAt":"2018-04-06T21:42:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 03, 2018 at 05:17:00PM +0200, Duy Nguyen wrote:\n\n> It's not that complex. With the EAGER_DEVELOPER patch removed, we can\n> have something like this where eager devs just need to put\n> \n> DEVOPTS = gentle no-suppression\n> \n> and you put\n> \n> DEVOPTS = gentle\n> \n> (bad naming, I didn't spend time thinking about names)\n\nIt seems to me like we're losing the point of DEVELOPER here. I thought\nthe idea was to have a turn-key flag you could set to get extra linting\non your commits. But now we're tweaking all kinds of individual options.\nAt some point are we better off just letting you put \"-Wno-foo\" in your\nCFLAGS yourself?\n\nI don't mind the version-based checks because they're automatic, so the\nfeature remains turn-key. But this kind of DEVOPTS seems like a step in\nthe wrong direction.\n\n-Peff\n"},{"id":"344061","messageId":"CACsJy8BOVnu+HXcVbkTKW6YZ8vQnwbAcaF4d2CzUbQLrZjuOJg@mail.gmail.com","threadId":"47961","inReplyTo":"20180406214236.GF7870@sigill.intra.peff.net","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-04-07T09:52:34Z","receivedAt":"2018-04-07T09:53:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Apr 6, 2018 at 11:42 PM, Jeff King wrote:\n> On Tue, Apr 03, 2018 at 05:17:00PM +0200, Duy Nguyen wrote:\n>\n>> It's not that complex. With the EAGER_DEVELOPER patch removed, we can\n>> have something like this where eager devs just need to put\n>>\n>> DEVOPTS = gentle no-suppression\n>>\n>> and you put\n>>\n>> DEVOPTS = gentle\n>>\n>> (bad naming, I didn't spend time thinking about names)\n>\n> It seems to me like we're losing the point of DEVELOPER here. I thought\n> the idea was to have a turn-key flag you could set to get extra linting\n> on your commits. But now we're tweaking all kinds of individual options.\n> At some point are we better off just letting you put \"-Wno-foo\" in your\n> CFLAGS yourself?\n\nIt's because what AEvar wanted is no longer a dev thing :)\n\n> I don't mind the version-based checks because they're automatic, so the\n> feature remains turn-key. But this kind of DEVOPTS seems like a step in\n> the wrong direction.\n>\n> -Peff\n\n\n\n-- \nDuy\n"},{"id":"344064","messageId":"87r2nryz0j.fsf@evledraar.gmail.com","threadId":"47961","inReplyTo":"CACsJy8BOVnu+HXcVbkTKW6YZ8vQnwbAcaF4d2CzUbQLrZjuOJg@mail.gmail.com","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-07T12:36:12Z","receivedAt":"2018-04-07T12:36:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Apr 07 2018, Duy Nguyen wrote:\n\n> On Fri, Apr 6, 2018 at 11:42 PM, Jeff King wrote:\n>> On Tue, Apr 03, 2018 at 05:17:00PM +0200, Duy Nguyen wrote:\n>>\n>>> It's not that complex. With the EAGER_DEVELOPER patch removed, we can\n>>> have something like this where eager devs just need to put\n>>>\n>>> DEVOPTS = gentle no-suppression\n>>>\n>>> and you put\n>>>\n>>> DEVOPTS = gentle\n>>>\n>>> (bad naming, I didn't spend time thinking about names)\n>>\n>> It seems to me like we're losing the point of DEVELOPER here. I thought\n>> the idea was to have a turn-key flag you could set to get extra linting\n>> on your commits. But now we're tweaking all kinds of individual options.\n>> At some point are we better off just letting you put \"-Wno-foo\" in your\n>> CFLAGS yourself?\n>\n> It's because what AEvar wanted is no longer a dev thing :)\n\nIt's still a dev thing, but just for a slightly different use-case. I\nhack my patches up with DEVELOPER=1, and then eventually deploy a\npatched git version with those (and others) on top of the latest release\ntag.\n\nAt that point I want the same compiler assertions, but might be building\non much older compilers, so I just want the warning output without\n-Werror.\n\nAnyway, I see you've pushed a new version with DEVOPTS. I'll submit mine\non top of that once your new version lands (unless you want to try to\nintegrate it yourself).\n"},{"id":"344071","messageId":"CACsJy8BNmKTweFiUgDtSfor7OYk1akzcPbLxadA7YfSZ56eCJQ@mail.gmail.com","threadId":"47961","inReplyTo":"87r2nryz0j.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-04-07T17:03:14Z","receivedAt":"2018-04-07T17:03:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 7, 2018 at 2:36 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Anyway, I see you've pushed a new version with DEVOPTS. I'll submit mine\n> on top of that once your new version lands (unless you want to try to\n> integrate it yourself).\n\nActually I think I'll just drop both EAGER_DEVELOPER and DEVOTPS.\n-- \nDuy\n"},{"id":"344073","messageId":"87po3azwsv.fsf@evledraar.gmail.com","threadId":"47961","inReplyTo":"CACsJy8BNmKTweFiUgDtSfor7OYk1akzcPbLxadA7YfSZ56eCJQ@mail.gmail.com","subject":"Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-07T18:38:40Z","receivedAt":"2018-04-07T18:39:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Apr 07 2018, Duy Nguyen wrote:\n\n> On Sat, Apr 7, 2018 at 2:36 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> Anyway, I see you've pushed a new version with DEVOPTS. I'll submit mine\n>> on top of that once your new version lands (unless you want to try to\n>> integrate it yourself).\n>\n> Actually I think I'll just drop both EAGER_DEVELOPER and DEVOTPS.\n\nOK. I'll submit something on top to optionally get rid of -Werror, since\nthat's the only bit I actually care about & have a use-case for.\n"},{"id":"344692","messageId":"20180414191946.30674-1-avarab@gmail.com","threadId":"47961","inReplyTo":"20180329150322.10722-1-pclouds@gmail.com","subject":"[PATCH v4 0/4] Make DEVELOPER more more flexible with DEVOPTS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-14T19:19:42Z","receivedAt":"2018-04-14T19:20:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is a v4 and replacement of gitster/nd/warn-more-for-devs. I'm\nsending this with Duy's blessing.\n\nThe first two patches are the same, except for one trivial\ns/faimily/family/ typo fix.\n\nThe third patch in gitster/nd/warn-more-for-devs (\"Makefile: add\nEAGER_DEVELOPER mode\") is gone, instead there's now a DEVOPTS\noption. The 3/4 and 4/4 add a way to turn off -Werror & the -Wextra\nsuppression, respectively.\n\nDuy was right in [1] that this is a much better and extensible way of\ndoing this than my \"Makefile: untangle DEVELOPER and -Werror\" patch.\n\nMost of 3/4 & 4/4 are just tweaked from git@github.com:pclouds/git.git\npclouds/more-warnings and combined with my previous 4/3 patch[2].\n\nI changed the \"no-suppression\" name in Duy's WIP patch to \"extra-all\",\nand \"gentle\" to \"no-error\". I think those are cleare,r and leave\nthings more open to future expansion, e.g. if we'd like pedantic-all.\n\n1. https://public-inbox.org/git/CACsJy8CyB0igY365NMkswSgAi9_rf+XBOMQyJ7XW6iQxQiCEyQ@mail.gmail.com/\n2. https://public-inbox.org/git/20180331164009.2264-1-avarab@gmail.com/\n\nNguyễn Thái Ngọc Duy (2):\n connect.c: mark die_initial_contact() NORETURN\n Makefile: detect compiler and enable more warnings in DEVELOPER=1\n\nÆvar Arnfjörð Bjarmason (2):\n Makefile: add a DEVOPTS to suppress -Werror under DEVELOPER\n Makefile: add a DEVOPTS to get all of -Wextra\n\n Makefile | 31 +++++++++++++++++++----------\n config.mak.dev | 42 +++++++++++++++++++++++++++++++++++++++\n connect.c | 2 +-\n detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 117 insertions(+), 11 deletions(-)\n create mode 100644 config.mak.dev\n create mode 100755 detect-compiler\n\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"344693","messageId":"20180414191946.30674-2-avarab@gmail.com","threadId":"47961","inReplyTo":"20180414191946.30674-1-avarab@gmail.com","subject":"[PATCH v4 1/4] connect.c: mark die_initial_contact() NORETURN","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-14T19:19:43Z","receivedAt":"2018-04-14T19:20:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThere is a series running in parallel with this one that adds code\nlike this\n\n switch (...) {\n case ...:\n die_initial_contact();\n case ...:\n\nThere is nothing wrong with this. There is no actual falling\nthrough. But since gcc is not that smart and gcc 7.x introduces\n-Wimplicit-fallthrough, it raises a false alarm in this case.\n\nThis class of warnings may be useful elsewhere, so instead of\nsuppressing the whole class, let's try to fix just this code. gcc is\nsmart enough to realize that no execution can continue after a\nNORETURN function call and no longer raises the warning.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n connect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex c3a014c5ba..49eca46462 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags)\n \treturn check_ref(ref->name, flags);\n }\n \n-static void die_initial_contact(int unexpected)\n+static NORETURN void die_initial_contact(int unexpected)\n {\n \tif (unexpected)\n \t\tdie(_(\"The remote end hung up upon initial contact\"));\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"344694","messageId":"20180414191946.30674-3-avarab@gmail.com","threadId":"47961","inReplyTo":"20180414191946.30674-1-avarab@gmail.com","subject":"[PATCH v4 2/4] Makefile: detect compiler and enable more warnings in DEVELOPER=1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-14T19:19:44Z","receivedAt":"2018-04-14T19:20:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThe set of extra warnings we enable when DEVELOPER has to be\nconservative because we can't assume any compiler version the\ndeveloper may use. Detect the compiler version so we know when it's\nsafe to enable -Wextra and maybe more.\n\nThese warning settings are mostly from my custom config.mak a long\ntime ago when I tried to enable as many warnings as possible that can\nstill build without showing warnings. Some of those warnings are\nprobably worth fixing instead of just suppressing in future.\n\nHelped-by: Jeff King \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Makefile | 15 +++++---------\n config.mak.dev | 38 +++++++++++++++++++++++++++++++++++\n detect-compiler | 53 +++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 96 insertions(+), 10 deletions(-)\n create mode 100644 config.mak.dev\n create mode 100755 detect-compiler\n\ndiff --git a/Makefile b/Makefile\nindex f181687250..3038c78325 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -441,6 +441,10 @@ all::\n #\n # When cross-compiling, define HOST_CPU as the canonical name of the CPU on\n # which the built Git will run (for instance \"x86_64\").\n+#\n+# Define DEVELOPER to enable more compiler warnings. Compiler version\n+# and family are auto detected, but could be overridden by defining\n+# COMPILER_FEATURES (see config.mak.dev)\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\n@@ -449,15 +453,6 @@ GIT-VERSION-FILE: FORCE\n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n CFLAGS = -g -O2 -Wall\n-DEVELOPER_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 LDFLAGS =\n ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n@@ -1062,7 +1057,7 @@ include config.mak.uname\n -include config.mak\n \n ifdef DEVELOPER\n-CFLAGS += $(DEVELOPER_CFLAGS)\n+include config.mak.dev\n endif\n \n comma := ,\ndiff --git a/config.mak.dev b/config.mak.dev\nnew file mode 100644\nindex 0000000000..716a14ecc7\n--- /dev/null\n+++ b/config.mak.dev\n@@ -0,0 +1,38 @@\n+CFLAGS += -Werror\n+CFLAGS += -Wdeclaration-after-statement\n+CFLAGS += -Wno-format-zero-length\n+CFLAGS += -Wold-style-definition\n+CFLAGS += -Woverflow\n+CFLAGS += -Wpointer-arith\n+CFLAGS += -Wstrict-prototypes\n+CFLAGS += -Wunused\n+CFLAGS += -Wvla\n+\n+ifndef COMPILER_FEATURES\n+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n+endif\n+\n+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wtautological-constant-out-of-range-compare\n+endif\n+\n+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n+CFLAGS += -Wextra\n+# if a function is public, there should be a prototype and the right\n+# header file should be included. If not, it should be static.\n+CFLAGS += -Wmissing-prototypes\n+# These are disabled because we have these all over the place.\n+CFLAGS += -Wno-empty-body\n+CFLAGS += -Wno-missing-field-initializers\n+CFLAGS += -Wno-sign-compare\n+CFLAGS += -Wno-unused-function\n+CFLAGS += -Wno-unused-parameter\n+endif\n+\n+# uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c\n+# not worth fixing since newer compilers correctly stop complaining\n+ifneq ($(filter gcc4,$(COMPILER_FEATURES)),)\n+ifeq ($(filter gcc5,$(COMPILER_FEATURES)),)\n+CFLAGS += -Wno-uninitialized\n+endif\n+endif\ndiff --git a/detect-compiler b/detect-compiler\nnew file mode 100755\nindex 0000000000..70b754481c\n--- /dev/null\n+++ b/detect-compiler\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+#\n+# Probe the compiler for vintage, version, etc. This is used for setting\n+# optional make knobs under the DEVELOPER knob.\n+\n+CC=\"$*\"\n+\n+# we get something like (this is at least true for gcc and clang)\n+#\n+# FreeBSD clang version 3.4.1 (tags/RELEASE...)\n+get_version_line() {\n+\t$CC -v 2>&1 | grep ' version '\n+}\n+\n+get_family() {\n+\tget_version_line | sed 's/^\\(.*\\) version [0-9][^ ]* .*/\\1/'\n+}\n+\n+get_version() {\n+\tget_version_line | sed 's/^.* version \\([0-9][^ ]*\\) .*/\\1/'\n+}\n+\n+print_flags() {\n+\tfamily=$1\n+\tversion=$(get_version | cut -f 1 -d .)\n+\n+\t# Print a feature flag not only for the current version, but also\n+\t# for any prior versions we encompass. This avoids needing to do\n+\t# numeric comparisons in make, which are awkward.\n+\twhile test \"$version\" -gt 0\n+\tdo\n+\t\techo $family$version\n+\t\tversion=$((version - 1))\n+\tdone\n+}\n+\n+case \"$(get_family)\" in\n+gcc)\n+\tprint_flags gcc\n+\t;;\n+clang)\n+\tprint_flags clang\n+\t;;\n+\"FreeBSD clang\")\n+\tprint_flags clang\n+\t;;\n+\"Apple LLVM\")\n+\tprint_flags clang\n+\t;;\n+*)\n+\t: unknown compiler family\n+\t;;\n+esac\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"344695","messageId":"20180414191946.30674-4-avarab@gmail.com","threadId":"47961","inReplyTo":"20180414191946.30674-1-avarab@gmail.com","subject":"[PATCH v4 3/4] Makefile: add a DEVOPTS to suppress -Werror under DEVELOPER","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-14T19:19:45Z","receivedAt":"2018-04-14T19:20:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a DEVOPTS variable that'll be used to tweak the behavior of\nDEVELOPER.\n\nI've long wanted to use DEVELOPER=1 in my production builds, but on\nsome old systems I still get warnings, and thus the build would\nfail. However if the build/tests fail for some other reason, it would\nstill be useful to scroll up and see what the relevant code is warning\nabout.\n\nThis change allows for that. Now setting DEVELOPER will set -Werror as\nbefore, but if DEVOPTS=no-error is provided is set you'll get the same\nwarnings, but without -Werror.\n\nHelped-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 10 ++++++++++\n config.mak.dev | 2 ++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 3038c78325..9a57495fae 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -445,6 +445,16 @@ all::\n # Define DEVELOPER to enable more compiler warnings. Compiler version\n # and family are auto detected, but could be overridden by defining\n # COMPILER_FEATURES (see config.mak.dev)\n+#\n+# When DEVELOPER is set, DEVOPTS can be used to control compiler\n+# options. This variable contains keywords separated by\n+# whitespace. The following keywords are are recognized:\n+#\n+# no-error:\n+#\n+# suppresses the -Werror that implicitly comes with\n+# DEVELOPER=1. Useful for getting the full set of errors\n+# without immediately dying, or for logging them.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 716a14ecc7..7a54426d78 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -1,4 +1,6 @@\n+ifeq ($(filter no-error,$(DEVOPTS)),)\n CFLAGS += -Werror\n+endif\n CFLAGS += -Wdeclaration-after-statement\n CFLAGS += -Wno-format-zero-length\n CFLAGS += -Wold-style-definition\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"344696","messageId":"20180414191946.30674-5-avarab@gmail.com","threadId":"47961","inReplyTo":"20180414191946.30674-1-avarab@gmail.com","subject":"[PATCH v4 4/4] Makefile: add a DEVOPTS to get all of -Wextra","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-14T19:19:46Z","receivedAt":"2018-04-14T19:20:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change DEVOPTS to understand a \"extra-all\" option. When the DEVELOPER\nflag is enabled we turn on -Wextra, but manually switch some of the\nwarnings it turns on off.\n\nThis is because we have many existing occurrences of them in the code\nbase. This mode will stop the suppression, let the developer see and\ndecide whether to fix them.\n\nThis change is a slight alteration of Nguyễn Thái Ngọc Duy\nEAGER_DEVELOPER mode patch[1]\n\n1. \"[PATCH v3 3/3] Makefile: add EAGER_DEVELOPER\n mode\" (<20180329150322.10722-4-pclouds@gmail.com>;\n https://public-inbox.org/git/20180329150322.10722-4-pclouds@gmail.com/)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 6 ++++++\n config.mak.dev | 2 ++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 9a57495fae..47ddc85aae 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -455,6 +455,12 @@ all::\n # suppresses the -Werror that implicitly comes with\n # DEVELOPER=1. Useful for getting the full set of errors\n # without immediately dying, or for logging them.\n+#\n+# extra-all:\n+#\n+# The DEVELOPER mode enables -Wextra with a few exceptions. By\n+# setting this flag the exceptions are removed, and all of\n+# -Wextra is used.\n \n GIT-VERSION-FILE: FORCE\n \t@$(SHELL_PATH) ./GIT-VERSION-GEN\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 7a54426d78..2d244ca470 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -23,6 +23,7 @@ CFLAGS += -Wextra\n # if a function is public, there should be a prototype and the right\n # header file should be included. If not, it should be static.\n CFLAGS += -Wmissing-prototypes\n+ifeq ($(filter extra-all,$(DEVOPTS)),)\n # These are disabled because we have these all over the place.\n CFLAGS += -Wno-empty-body\n CFLAGS += -Wno-missing-field-initializers\n@@ -30,6 +31,7 @@ CFLAGS += -Wno-sign-compare\n CFLAGS += -Wno-unused-function\n CFLAGS += -Wno-unused-parameter\n endif\n+endif\n \n # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c\n # not worth fixing since newer compilers correctly stop complaining\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"344772","messageId":"xmqqin8rycie.fsf@gitster-ct.c.googlers.com","threadId":"47961","inReplyTo":"20180414191946.30674-1-avarab@gmail.com","subject":"Re: [PATCH v4 0/4] Make DEVELOPER more more flexible with DEVOPTS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-16T04:57:13Z","receivedAt":"2018-04-16T04:57:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This is a v4 and replacement of gitster/nd/warn-more-for-devs. I'm\n> sending this with Duy's blessing.\n>\n> The first two patches are the same, except for one trivial\n> s/faimily/family/ typo fix.\n>\n> The third patch in gitster/nd/warn-more-for-devs (\"Makefile: add\n> EAGER_DEVELOPER mode\") is gone, instead there's now a DEVOPTS\n> option. The 3/4 and 4/4 add a way to turn off -Werror & the -Wextra\n> suppression, respectively.\n\nLooks sensible, mostly. Will queue. Thanks.\n"}]}