{"thread":{"id":"56597","subject":"[PATCH 0/3] Makefile: tighten pedantic flag","startedAt":"2021-09-28T09:11:09Z","lastAt":"2021-09-29T04:08:43Z","messageCount":13,"participants":["Carlo Marcelo Arenas Belón","Ævar Arnfjörð Bjarmason","Carlo Arenas","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"437269","messageId":"20210928091054.78895-1-carenas@gmail.com","threadId":"56597","inReplyTo":null,"subject":"[PATCH 0/3] Makefile: tighten pedantic flag","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-28T09:10:51Z","receivedAt":"2021-09-28T09:11:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series uses the feedback provided so far to tighten the pedantic\nflags that were added previously, which is mostly done in the first\npatch.\n\nIt is based on js/win-lazyload-buildfix to avoid merge conflicts with\nit, but could be also applied independently if needed (specially patches\n2 and 3 that are somehow orthogonal cleanups).\n\nAlternatively, patch 3 could be dropped and patch 1 refactor based on\nthe reviewer feedback.\n\nCarlo Marcelo Arenas Belón (3):\n  Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better\n  Makefile: avoid multiple -Wall in CFLAGS\n  config.mak.dev: simplify compiler check for multiple compilers\n\n Makefile         | 3 ++-\n config.mak.dev   | 9 ++++++---\n config.mak.uname | 3 +++\n 3 files changed, 11 insertions(+), 4 deletions(-)\n\n\nbase-commit: 2d84c4ed571215f4cdd5ea05a46861974d10d123\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437270","messageId":"20210928091054.78895-2-carenas@gmail.com","threadId":"56597","inReplyTo":"20210928091054.78895-1-carenas@gmail.com","subject":"[PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-28T09:10:52Z","receivedAt":"2021-09-28T09:11:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"6a8cbc41ba (developer: enable pedantic by default, 2021-09-03)\nenables pedantic mode in as many compilers as possible to help gather\nfeedback on future tightening of the net, so lets do so.\n\n-Wpedantic is missing in some really old gcc 4 versions so lets restrict\nit to gcc5 and clang4 (it does work in clang3 AFAIK, but it will be\nunlikely a developer will use such an old compiler anyway).\n\nMinGW gcc is the only one which has -Wno-pedantic-ms-format, and while\nthat is available also in older compilers, the Windows SDK provides gcc10\nso lets aim for that.  Note that in order to target the flag to only\nWindows, additional changes were needed in config.mak.uname to propagate\nthe OS detection done there.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n config.mak.dev   | 6 +++++-\n config.mak.uname | 3 +++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex cdf043c52b..c81be62a5c 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -9,11 +9,15 @@ endif\n DEVELOPER_CFLAGS += -Wall\n ifeq ($(filter no-pedantic,$(DEVOPTS)),)\n DEVELOPER_CFLAGS += -pedantic\n+ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)\n DEVELOPER_CFLAGS += -Wpedantic\n-ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)\n+ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)\n+ifeq ($(uname_S),MINGW)\n DEVELOPER_CFLAGS += -Wno-pedantic-ms-format\n endif\n endif\n+endif\n+endif\n DEVELOPER_CFLAGS += -Wdeclaration-after-statement\n DEVELOPER_CFLAGS += -Wformat-security\n DEVELOPER_CFLAGS += -Wold-style-definition\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 76516aaa9a..aa68bbdec7 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -589,6 +589,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tSHELL_PATH = /usr/coreutils/bin/bash\n endif\n ifneq (,$(findstring MINGW,$(uname_S)))\n+\tuname_S := MINGW\n \tpathsep = ;\n \tHAVE_ALLOCA_H = YesPlease\n \tNO_PREAD = YesPlease\n@@ -705,6 +706,8 @@ ifeq ($(uname_S),QNX)\n \tNO_STRLCPY = YesPlease\n endif\n \n+export uname_S\n+\n vcxproj:\n \t# Require clean work tree\n \tgit update-index -q --refresh && \\\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437271","messageId":"20210928091054.78895-3-carenas@gmail.com","threadId":"56597","inReplyTo":"20210928091054.78895-1-carenas@gmail.com","subject":"[PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-28T09:10:53Z","receivedAt":"2021-09-28T09:11:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help\nautoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to\nworkaround the lack of one from config.mak.autogen.\n\nSince 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and\nCFLAGS=\"...\", 2019-02-22), that variable is set instead as part of\nDEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so\nit can be safely removed from config.mak.dev if set instead in the\nMakefile.\n\nThis also has the advantage of separating cleanly CFLAGS which are\nused for building with the ones that provide with diagnostics.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n Makefile       | 3 ++-\n config.mak.dev | 1 -\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 9df565f27b..963b9e7c6b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1200,7 +1200,8 @@ endif\n # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be\n # tweaked by config.* below as well as the command-line, both of\n # which'll override these defaults.\n-CFLAGS = -g -O2 -Wall\n+CFLAGS = -g -O2\n+DEVELOPER_CFLAGS = -Wall\n LDFLAGS =\n CC_LD_DYNPATH = -Wl,-rpath,\n BASIC_CFLAGS = -I.\ndiff --git a/config.mak.dev b/config.mak.dev\nindex c81be62a5c..90c47d2782 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),)\n DEVELOPER_CFLAGS += -Werror\n SPARSE_FLAGS += -Wsparse-error\n endif\n-DEVELOPER_CFLAGS += -Wall\n ifeq ($(filter no-pedantic,$(DEVOPTS)),)\n DEVELOPER_CFLAGS += -pedantic\n ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)\n-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437272","messageId":"20210928091054.78895-4-carenas@gmail.com","threadId":"56597","inReplyTo":"20210928091054.78895-1-carenas@gmail.com","subject":"[PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-28T09:10:54Z","receivedAt":"2021-09-28T09:11:18Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"1da1580e4c (Makefile: detect compiler and enable more warnings in\nDEVELOPER=1, 2018-04-14), includes an $(or) of two different filters\nto check for both gcc and clang versions.\n\nAs shown in a previous patch, a simpler syntax is available so apply\nthe same logic here also for consistency.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n config.mak.dev | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 90c47d2782..b66fae8665 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -31,7 +31,7 @@ ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n endif\n \n-ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n+ifneq ($(filter clang4 gcc6,$(COMPILER_FEATURES)),)\n DEVELOPER_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-- \n2.33.0.955.gee03ddbf0e\n\n"},{"id":"437273","messageId":"87wnn1vxh8.fsf@evledraar.gmail.com","threadId":"56597","inReplyTo":"20210928091054.78895-3-carenas@gmail.com","subject":"Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T09:19:24Z","receivedAt":"2021-09-28T09:52:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote:\n\n> 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help\n> autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to\n> workaround the lack of one from config.mak.autogen.\n>\n> Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and\n> CFLAGS=\"...\", 2019-02-22), that variable is set instead as part of\n> DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so\n> it can be safely removed from config.mak.dev if set instead in the\n> Makefile.\n>\n> This also has the advantage of separating cleanly CFLAGS which are\n> used for building with the ones that provide with diagnostics.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  Makefile       | 3 ++-\n>  config.mak.dev | 1 -\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 9df565f27b..963b9e7c6b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1200,7 +1200,8 @@ endif\n>  # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be\n>  # tweaked by config.* below as well as the command-line, both of\n>  # which'll override these defaults.\n> -CFLAGS = -g -O2 -Wall\n> +CFLAGS = -g -O2\n> +DEVELOPER_CFLAGS = -Wall\n>  LDFLAGS =\n>  CC_LD_DYNPATH = -Wl,-rpath,\n>  BASIC_CFLAGS = -I.\n> diff --git a/config.mak.dev b/config.mak.dev\n> index c81be62a5c..90c47d2782 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),)\n>  DEVELOPER_CFLAGS += -Werror\n>  SPARSE_FLAGS += -Wsparse-error\n>  endif\n> -DEVELOPER_CFLAGS += -Wall\n>  ifeq ($(filter no-pedantic,$(DEVOPTS)),)\n>  DEVELOPER_CFLAGS += -pedantic\n>  ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)\n\nThis really breaks things for anyone who's relying on specifing CFLAGS\nnow to clobber the default -Wall configuration, e.g. on both xlc and aCC\nafter this:\n\n    xlc: 1501-289 (W) Option -Wall was incorrectly specified. The option will be ignored.\n    cc: error 1914: bad form for `-W' option\n\nI.e. they didn't work before, but I've got CFLAGS=\"-g -O0\" for both in\nmy build scripts, so they didn't get -Wall before, but now they do, so\nI'll need:\n\n    CFLAGS=<that> DEVELOPER_CFLAGS=\n\nAnd in my own dev setup I do in config.mak: \"CFLAGS=-g -O0\", and then\nrely on config.mak.dev to set -Wall, but if I did e.g.:\n\n    DEVELOPER= CFLAGS=\"-g -O0 -Wall\"\n\nI'd end up with -Wall twice, gcc doesn't complain, but maybe some other\ntoolchains do.\n\nFor the former case this seems like a really odd and leaky interface\nsince I don't have DEVELOPER=1. Let's leave DEVOPTS, DEVELOPER_CFLAGS\netc. etc. only in effect for anyone who turns on that option.\n\nAnyway, I don't think it's a no-go to make a change in this direction,\nand while it would break builds for some perhaps the end result is worth\nit. I haven't really looked closely enough at the Makefile logic you're\nuntangling to form an opinion on it.\n\nBut I think this needs to at least have\ns/DEVELOPER_CFLAGS/WARNING_CFLAGS/g or something.\n\nBut not to saddle you with an impossible task, wouldn't this whole thing\nbe much easier if we included config.mak* before setting our own CFLAGS\netc. defaults? But of course that would break for anyone relying on \"+=\"\nworking, so I don't know.\n"},{"id":"437277","messageId":"CAPUEspj_hOXRc2d+c+DTvShgWX8NH+fXKD4Pk+_G=nj9Z97VnQ@mail.gmail.com","threadId":"56597","inReplyTo":"87wnn1vxh8.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-28T11:03:09Z","receivedAt":"2021-09-28T11:03:23Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Sep 28, 2021 at 2:52 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> This really breaks things for anyone who's relying on specifing CFLAGS\n> now to clobber the default -Wall configuration, e.g. on both xlc and aCC\n> after this:\n\ngot it; then it should have been DEVELOPER_CFLAGS+=-Wall the one to\nkeep, and that way there is no need to hack CFLAGS just to disable\n-Wall in non GNU compilers IMHO\nof course, that still leaves the question if -Wall is still something\nwe want to have by default for people NOT using DEVELOPER=1 to\ncompile.\n\n> I'd end up with -Wall twice, gcc doesn't complain, but maybe some other\n> toolchains do.\n\nclang wouldn't complain either, and indeed you already have 2 -Wall,\nwhich is what I was trying to clean up.\n\n> But I think this needs to at least have\n> s/DEVELOPER_CFLAGS/WARNING_CFLAGS/g or something.\n\nmakes sense, even though I assumed there would be less churn if\nreusing the variable you came up with.\nin any case, I think it is clear that this change that I thought will\nbe tiny might be better tackled in a different thread, not to\ncomplicate the main reason for this thread, which was to make sure\nthat users that might build the next release (even with DEVELOPER=1)\nare less likely to find issues than the developers that got their\nfirst taste of \"pedantic\" in master.\n\nwill wait for more feedback before doing a reroll, but would be nice\nto get feedback also in the other 2 patches, which hopefully are less\nproblematic.\n\nCarlo\n"},{"id":"437278","messageId":"87o88cx69w.fsf@evledraar.gmail.com","threadId":"56597","inReplyTo":"20210928091054.78895-2-carenas@gmail.com","subject":"Re: [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T11:46:14Z","receivedAt":"2021-09-28T11:56:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote:\n\n> 6a8cbc41ba (developer: enable pedantic by default, 2021-09-03)\n> enables pedantic mode in as many compilers as possible to help gather\n> feedback on future tightening of the net, so lets do so.\n>\n> -Wpedantic is missing in some really old gcc 4 versions so lets restrict\n> it to gcc5 and clang4 (it does work in clang3 AFAIK, but it will be\n> unlikely a developer will use such an old compiler anyway).\n>\n> MinGW gcc is the only one which has -Wno-pedantic-ms-format, and while\n> that is available also in older compilers, the Windows SDK provides gcc10\n> so lets aim for that.  Note that in order to target the flag to only\n> Windows, additional changes were needed in config.mak.uname to propagate\n> the OS detection done there.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  config.mak.dev   | 6 +++++-\n>  config.mak.uname | 3 +++\n>  2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/config.mak.dev b/config.mak.dev\n> index cdf043c52b..c81be62a5c 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -9,11 +9,15 @@ endif\n>  DEVELOPER_CFLAGS += -Wall\n>  ifeq ($(filter no-pedantic,$(DEVOPTS)),)\n>  DEVELOPER_CFLAGS += -pedantic\n> +ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)\n>  DEVELOPER_CFLAGS += -Wpedantic\n> -ifneq ($(filter gcc5,$(COMPILER_FEATURES)),)\n> +ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)\n> +ifeq ($(uname_S),MINGW)\n>  DEVELOPER_CFLAGS += -Wno-pedantic-ms-format\n>  endif\n>  endif\n> +endif\n> +endif\n>  DEVELOPER_CFLAGS += -Wdeclaration-after-statement\n>  DEVELOPER_CFLAGS += -Wformat-security\n>  DEVELOPER_CFLAGS += -Wold-style-definition\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 76516aaa9a..aa68bbdec7 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -589,6 +589,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n>  \tSHELL_PATH = /usr/coreutils/bin/bash\n>  endif\n>  ifneq (,$(findstring MINGW,$(uname_S)))\n> +\tuname_S := MINGW\n\nJust in terms of implementation, this somewhat pre-dates your change,\nbut every single other uname we check as a constant. I wonder if this\n\"findstring\" is really needed under MINGW. It was added in f4626df51f6\n(Add target architecture MinGW., 2007-12-01).\n\n(Goes and seaches stackoverflow etc.)\n\nAh yes, it seems it'll emit e.g. \"MINGW64_NT-10.0\", ew!\n\nIn any case, I wonder if we should at least be better off with the\ndiff-at-the-end on top (untested).\n\nAnd also not necessarily for this series, but IMO this sort of thing\nreally longer-term belongs in config.mak.uname (or maybe a\nconfig.mak.dev.uname, ew!). Well, maybe. Anyway, looking at potentially\nimplementing that we get into similar ordering issues as I noted in my\n2/3 comment, i.e. we'd have to hoist \"COMPILER_FEATURES\" over to the\nmain Makefile before including both.\n\nSo nevermind I guess, but aside from which variable we set/override\nwhere (and feel very free to ignore my musings there) this change LGTM.\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex aa68bbdec72..0028891ac67 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -11,6 +11,10 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')\n uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')\n uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')\n \n+ifneq (,$(findstring MINGW,$(uname_S)))\n+\tuname_S := MINGW\n+endif\n+\n ifdef MSVC\n \t# avoid the MingW and Cygwin configuration sections\n \tuname_S := Windows\n@@ -588,8 +592,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n \tSHELL_PATH = /usr/coreutils/bin/bash\n endif\n-ifneq (,$(findstring MINGW,$(uname_S)))\n-\tuname_S := MINGW\n+ifeq ($(uname_S),MINGW)\n \tpathsep = ;\n \tHAVE_ALLOCA_H = YesPlease\n \tNO_PREAD = YesPlease\n"},{"id":"437279","messageId":"87k0j0x5mg.fsf@evledraar.gmail.com","threadId":"56597","inReplyTo":"20210928091054.78895-4-carenas@gmail.com","subject":"Re: [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T12:07:09Z","receivedAt":"2021-09-28T12:10:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote:\n\n> 1da1580e4c (Makefile: detect compiler and enable more warnings in\n> DEVELOPER=1, 2018-04-14), includes an $(or) of two different filters\n> to check for both gcc and clang versions.\n>\n> As shown in a previous patch, a simpler syntax is available so apply\n> the same logic here also for consistency.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  config.mak.dev | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/config.mak.dev b/config.mak.dev\n> index 90c47d2782..b66fae8665 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -31,7 +31,7 @@ ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n>  DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n>  endif\n>  \n> -ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)\n> +ifneq ($(filter clang4 gcc6,$(COMPILER_FEATURES)),)\n>  DEVELOPER_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\nThis looks like a good cleanup and ends up being much more readable.\n\nI wonder if eventually a larger change to simplify this like perhaps the\nbelow wouldn't be nicer. I didn't test it carefully & may have gotten\nthe logic wrong, which I think somewhat makes the point that reading\nthis ifeq/ifneq logic (especially the nested bit at the end) is a bit\nhard, at least for me:)\n\nAnyway, feel free to ignore the below, and I think it's certainly not\nneeded for this series, just my 0.02 if you're eventually refactoring\nsome of this.\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 0a87d8cbe9d..df27340b4b0 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -2,6 +2,14 @@ ifndef COMPILER_FEATURES\n COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n endif\n \n+# These are all the empty string if the compiler *isn't* X or\n+# earlier. Note that clang v5, v6 etc. also qualify as \"have v4\".\n+CC_HAVE_CLANG4 = $(filter clang4,$(COMPILER_FEATURES))\n+CC_HAVE_GCC4 = $(filter gcc4,$(COMPILER_FEATURES))\n+CC_HAVE_GCC5 = $(filter gcc5,$(COMPILER_FEATURES))\n+CC_HAVE_GCC6 = $(filter gcc6,$(COMPILER_FEATURES))\n+CC_HAVE_GCC10 = $(filter gcc10,$(COMPILER_FEATURES))\n+\n ifeq ($(filter no-error,$(DEVOPTS)),)\n DEVELOPER_CFLAGS += -Werror\n SPARSE_FLAGS += -Wsparse-error\n@@ -9,9 +17,9 @@ endif\n DEVELOPER_CFLAGS += -Wall\n ifeq ($(filter no-pedantic,$(DEVOPTS)),)\n DEVELOPER_CFLAGS += -pedantic\n-ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)\n+ifneq ($(CC_HAVE_CLANG4)$(CC_HAVE_GCC5),)\n DEVELOPER_CFLAGS += -Wpedantic\n-ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)\n+ifneq ($(CC_HAVE_GCC10),)\n ifeq ($(uname_S),MINGW)\n DEVELOPER_CFLAGS += -Wno-pedantic-ms-format\n DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types\n@@ -29,11 +37,11 @@ DEVELOPER_CFLAGS += -Wunused\n DEVELOPER_CFLAGS += -Wvla\n DEVELOPER_CFLAGS += -fno-common\n \n-ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n+ifneq ($(CC_HAVE_CLANG4),)\n DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n endif\n \n-ifneq ($(filter clang4 gcc6,$(COMPILER_FEATURES)),)\n+ifneq ($(CC_HAVE_CLANG4)$(CC_HAVE_GCC6),)\n DEVELOPER_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@@ -49,8 +57,8 @@ 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+ifneq ($(CC_HAVE_GCC4),)\n+ifeq ($(CC_HAVE_GCC5),)\n DEVELOPER_CFLAGS += -Wno-uninitialized\n endif\n endif\n"},{"id":"437366","messageId":"xmqqbl4cjt3e.fsf@gitster.g","threadId":"56597","inReplyTo":"20210928091054.78895-3-carenas@gmail.com","subject":"Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-28T21:19:49Z","receivedAt":"2021-09-28T21:19:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help\n> autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to\n> workaround the lack of one from config.mak.autogen.\n>\n> Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and\n> CFLAGS=\"...\", 2019-02-22), that variable is set instead as part of\n> DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so\n> it can be safely removed from config.mak.dev if set instead in the\n> Makefile.\n\nHmph, don't this break non-developers, though?\n\nThey now do not get -Wall that they used to?  Or am I reading the\npatch incorrectly?\n\nThanks.\n\n> This also has the advantage of separating cleanly CFLAGS which are\n> used for building with the ones that provide with diagnostics.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  Makefile       | 3 ++-\n>  config.mak.dev | 1 -\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 9df565f27b..963b9e7c6b 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1200,7 +1200,8 @@ endif\n>  # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be\n>  # tweaked by config.* below as well as the command-line, both of\n>  # which'll override these defaults.\n> -CFLAGS = -g -O2 -Wall\n> +CFLAGS = -g -O2\n> +DEVELOPER_CFLAGS = -Wall\n>  LDFLAGS =\n>  CC_LD_DYNPATH = -Wl,-rpath,\n>  BASIC_CFLAGS = -I.\n> diff --git a/config.mak.dev b/config.mak.dev\n> index c81be62a5c..90c47d2782 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),)\n>  DEVELOPER_CFLAGS += -Werror\n>  SPARSE_FLAGS += -Wsparse-error\n>  endif\n> -DEVELOPER_CFLAGS += -Wall\n>  ifeq ($(filter no-pedantic,$(DEVOPTS)),)\n>  DEVELOPER_CFLAGS += -pedantic\n>  ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)\n"},{"id":"437367","messageId":"xmqq4ka4jsmd.fsf@gitster.g","threadId":"56597","inReplyTo":"87k0j0x5mg.fsf@evledraar.gmail.com","subject":"Re: [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-28T21:30:02Z","receivedAt":"2021-09-28T21:30:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Anyway, feel free to ignore the below, and I think it's certainly not\n> needed for this series, just my 0.02 if you're eventually refactoring\n> some of this.\n>\n> diff --git a/config.mak.dev b/config.mak.dev\n> index 0a87d8cbe9d..df27340b4b0 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -2,6 +2,14 @@ ifndef COMPILER_FEATURES\n>  COMPILER_FEATURES := $(shell ./detect-compiler $(CC))\n>  endif\n>  \n> +# These are all the empty string if the compiler *isn't* X or\n> +# earlier. Note that clang v5, v6 etc. also qualify as \"have v4\".\n\nI had to read this three times and still cannot decide what it wants\not say.\n\n> +CC_HAVE_CLANG4 = $(filter clang4,$(COMPILER_FEATURES))\n> +CC_HAVE_GCC4 = $(filter gcc4,$(COMPILER_FEATURES))\n> +CC_HAVE_GCC5 = $(filter gcc5,$(COMPILER_FEATURES))\n> +CC_HAVE_GCC6 = $(filter gcc6,$(COMPILER_FEATURES))\n> +CC_HAVE_GCC10 = $(filter gcc10,$(COMPILER_FEATURES))\n\nIt is empty if the compiler isn't X.  It is also empty if the\ncompiler isn't earlier than X?  That would mean version (X+1) would\nqualify, as it is not X, which is not what we want.\n\nI think you meant \"... empty string, unless the compiler is X or\nlater.\"\n\nAlso, I often see \"Note that\" to imply \"despite what I just said\",\nbut as far as I can tell CLANG4 is not all that special and follows\nthe same general rule.  Perhaps \"Note that\" -> \"For example,\" is\nneeded to clarify.\n\nHaving said all that, I find that the original \n\n\tifneq ($(filter x y, $(COMPILER_FEATURES)),)\n\nidiom is readable enough, and\n\n\tifneq ($(CC_HAVE_X)$(CC_HAVE_Y),)\n\ndoes not necessarily make it easier to spot X and Y that are being\nchecked with the conditional.\n\n>  ifeq ($(filter no-error,$(DEVOPTS)),)\n>  DEVELOPER_CFLAGS += -Werror\n>  SPARSE_FLAGS += -Wsparse-error\n> @@ -9,9 +17,9 @@ endif\n>  DEVELOPER_CFLAGS += -Wall\n>  ifeq ($(filter no-pedantic,$(DEVOPTS)),)\n>  DEVELOPER_CFLAGS += -pedantic\n> -ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),)\n> +ifneq ($(CC_HAVE_CLANG4)$(CC_HAVE_GCC5),)\n"},{"id":"437377","messageId":"CAPUEspjvZHEb0X71tFRuFJoxK9eChajfC40OVw7iaup1R+1jZg@mail.gmail.com","threadId":"56597","inReplyTo":"xmqqbl4cjt3e.fsf@gitster.g","subject":"Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-28T23:22:54Z","receivedAt":"2021-09-28T23:23:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Sep 28, 2021 at 2:19 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n>\n> > 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help\n> > autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to\n> > workaround the lack of one from config.mak.autogen.\n> >\n> > Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and\n> > CFLAGS=\"...\", 2019-02-22), that variable is set instead as part of\n> > DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so\n> > it can be safely removed from config.mak.dev if set instead in the\n> > Makefile.\n>\n> Hmph, don't this break non-developers, though?\n>\n> They now do not get -Wall that they used to?  Or am I reading the\n> patch incorrectly?\n\nIt is subtle and apparently problematic as some users[1] rely on\nremoving -Wall to support non GNU compilers, but my change kept the\n-Wall by default by making sure it got set in DEVELOPER_FLAGS instead\n(which as Ævar points out might be a layering violation and hence\nmight need more planning).\n\nEither way I'll be dropping this series for now, to make sure that the\nminimal change is put forward as part of js/win-lazyload-buildfix\ninstead.\n\nCarlo\n\n[1] https://lore.kernel.org/git/CAPUEspj_hOXRc2d+c+DTvShgWX8NH+fXKD4Pk+_G=nj9Z97VnQ@mail.gmail.com/\n"},{"id":"437390","messageId":"xmqq1r58i82p.fsf@gitster.g","threadId":"56597","inReplyTo":"87o88cx69w.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-28T23:39:10Z","receivedAt":"2021-09-28T23:39:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> In any case, I wonder if we should at least be better off with the\n> diff-at-the-end on top (untested).\n> ...\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -11,6 +11,10 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not')\n>  uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')\n>  uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not')\n>  \n> +ifneq (,$(findstring MINGW,$(uname_S)))\n> +\tuname_S := MINGW\n> +endif\n> +\n\nIt does sound like a better organization to \"normalize\" different\nspellings early so that later users of the macro can pretend that\nthere is no \"MINGW64_BLA-foo\" to worry about.\n\n>  ifdef MSVC\n>  \t# avoid the MingW and Cygwin configuration sections\n>  \tuname_S := Windows\n> @@ -588,8 +592,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)\n>  \tSANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin\n>  \tSHELL_PATH = /usr/coreutils/bin/bash\n>  endif\n> -ifneq (,$(findstring MINGW,$(uname_S)))\n> -\tuname_S := MINGW\n> +ifeq ($(uname_S),MINGW)\n>  \tpathsep = ;\n>  \tHAVE_ALLOCA_H = YesPlease\n>  \tNO_PREAD = YesPlease\n\n\nThanks.\n"},{"id":"437429","messageId":"xmqqo88cf2go.fsf@gitster.g","threadId":"56597","inReplyTo":"CAPUEspjvZHEb0X71tFRuFJoxK9eChajfC40OVw7iaup1R+1jZg@mail.gmail.com","subject":"Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T04:08:39Z","receivedAt":"2021-09-29T04:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> Either way I'll be dropping this series for now, to make sure that the\n> minimal change is put forward as part of js/win-lazyload-buildfix\n> instead.\n\nThanks.\n"}]}