* [PATCH 0/3] Makefile: tighten pedantic flag
@ 2021-09-28 9:10 Carlo Marcelo Arenas Belón
2021-09-28 9:10 ` [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better Carlo Marcelo Arenas Belón
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2021-09-28 9:10 UTC (permalink / raw)
To: git
Cc: t.gummerer, avarab, pclouds, jonathantanmy,
Carlo Marcelo Arenas Belón
This series uses the feedback provided so far to tighten the pedantic
flags that were added previously, which is mostly done in the first
patch.
It is based on js/win-lazyload-buildfix to avoid merge conflicts with
it, but could be also applied independently if needed (specially patches
2 and 3 that are somehow orthogonal cleanups).
Alternatively, patch 3 could be dropped and patch 1 refactor based on
the reviewer feedback.
Carlo Marcelo Arenas Belón (3):
Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better
Makefile: avoid multiple -Wall in CFLAGS
config.mak.dev: simplify compiler check for multiple compilers
Makefile | 3 ++-
config.mak.dev | 9 ++++++---
config.mak.uname | 3 +++
3 files changed, 11 insertions(+), 4 deletions(-)
base-commit: 2d84c4ed571215f4cdd5ea05a46861974d10d123
--
2.33.0.955.gee03ddbf0e
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better 2021-09-28 9:10 [PATCH 0/3] Makefile: tighten pedantic flag Carlo Marcelo Arenas Belón @ 2021-09-28 9:10 ` Carlo Marcelo Arenas Belón 2021-09-28 11:46 ` Ævar Arnfjörð Bjarmason 2021-09-28 9:10 ` [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS Carlo Marcelo Arenas Belón 2021-09-28 9:10 ` [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers Carlo Marcelo Arenas Belón 2 siblings, 1 reply; 13+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-28 9:10 UTC (permalink / raw) To: git Cc: t.gummerer, avarab, pclouds, jonathantanmy, Carlo Marcelo Arenas Belón 6a8cbc41ba (developer: enable pedantic by default, 2021-09-03) enables pedantic mode in as many compilers as possible to help gather feedback on future tightening of the net, so lets do so. -Wpedantic is missing in some really old gcc 4 versions so lets restrict it to gcc5 and clang4 (it does work in clang3 AFAIK, but it will be unlikely a developer will use such an old compiler anyway). MinGW gcc is the only one which has -Wno-pedantic-ms-format, and while that is available also in older compilers, the Windows SDK provides gcc10 so lets aim for that. Note that in order to target the flag to only Windows, additional changes were needed in config.mak.uname to propagate the OS detection done there. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- config.mak.dev | 6 +++++- config.mak.uname | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/config.mak.dev b/config.mak.dev index cdf043c52b..c81be62a5c 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -9,11 +9,15 @@ endif DEVELOPER_CFLAGS += -Wall ifeq ($(filter no-pedantic,$(DEVOPTS)),) DEVELOPER_CFLAGS += -pedantic +ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wpedantic -ifneq ($(filter gcc5,$(COMPILER_FEATURES)),) +ifneq ($(filter gcc10,$(COMPILER_FEATURES)),) +ifeq ($(uname_S),MINGW) DEVELOPER_CFLAGS += -Wno-pedantic-ms-format endif endif +endif +endif DEVELOPER_CFLAGS += -Wdeclaration-after-statement DEVELOPER_CFLAGS += -Wformat-security DEVELOPER_CFLAGS += -Wold-style-definition diff --git a/config.mak.uname b/config.mak.uname index 76516aaa9a..aa68bbdec7 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -589,6 +589,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) SHELL_PATH = /usr/coreutils/bin/bash endif ifneq (,$(findstring MINGW,$(uname_S))) + uname_S := MINGW pathsep = ; HAVE_ALLOCA_H = YesPlease NO_PREAD = YesPlease @@ -705,6 +706,8 @@ ifeq ($(uname_S),QNX) NO_STRLCPY = YesPlease endif +export uname_S + vcxproj: # Require clean work tree git update-index -q --refresh && \ -- 2.33.0.955.gee03ddbf0e ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better 2021-09-28 9:10 ` [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better Carlo Marcelo Arenas Belón @ 2021-09-28 11:46 ` Ævar Arnfjörð Bjarmason 2021-09-28 23:39 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-09-28 11:46 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, t.gummerer, pclouds, jonathantanmy On Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote: > 6a8cbc41ba (developer: enable pedantic by default, 2021-09-03) > enables pedantic mode in as many compilers as possible to help gather > feedback on future tightening of the net, so lets do so. > > -Wpedantic is missing in some really old gcc 4 versions so lets restrict > it to gcc5 and clang4 (it does work in clang3 AFAIK, but it will be > unlikely a developer will use such an old compiler anyway). > > MinGW gcc is the only one which has -Wno-pedantic-ms-format, and while > that is available also in older compilers, the Windows SDK provides gcc10 > so lets aim for that. Note that in order to target the flag to only > Windows, additional changes were needed in config.mak.uname to propagate > the OS detection done there. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > config.mak.dev | 6 +++++- > config.mak.uname | 3 +++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/config.mak.dev b/config.mak.dev > index cdf043c52b..c81be62a5c 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -9,11 +9,15 @@ endif > DEVELOPER_CFLAGS += -Wall > ifeq ($(filter no-pedantic,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -pedantic > +ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) > DEVELOPER_CFLAGS += -Wpedantic > -ifneq ($(filter gcc5,$(COMPILER_FEATURES)),) > +ifneq ($(filter gcc10,$(COMPILER_FEATURES)),) > +ifeq ($(uname_S),MINGW) > DEVELOPER_CFLAGS += -Wno-pedantic-ms-format > endif > endif > +endif > +endif > DEVELOPER_CFLAGS += -Wdeclaration-after-statement > DEVELOPER_CFLAGS += -Wformat-security > DEVELOPER_CFLAGS += -Wold-style-definition > diff --git a/config.mak.uname b/config.mak.uname > index 76516aaa9a..aa68bbdec7 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -589,6 +589,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > SHELL_PATH = /usr/coreutils/bin/bash > endif > ifneq (,$(findstring MINGW,$(uname_S))) > + uname_S := MINGW Just in terms of implementation, this somewhat pre-dates your change, but every single other uname we check as a constant. I wonder if this "findstring" is really needed under MINGW. It was added in f4626df51f6 (Add target architecture MinGW., 2007-12-01). (Goes and seaches stackoverflow etc.) Ah yes, it seems it'll emit e.g. "MINGW64_NT-10.0", ew! In any case, I wonder if we should at least be better off with the diff-at-the-end on top (untested). And also not necessarily for this series, but IMO this sort of thing really longer-term belongs in config.mak.uname (or maybe a config.mak.dev.uname, ew!). Well, maybe. Anyway, looking at potentially implementing that we get into similar ordering issues as I noted in my 2/3 comment, i.e. we'd have to hoist "COMPILER_FEATURES" over to the main Makefile before including both. So nevermind I guess, but aside from which variable we set/override where (and feel very free to ignore my musings there) this change LGTM. diff --git a/config.mak.uname b/config.mak.uname index aa68bbdec72..0028891ac67 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -11,6 +11,10 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not') uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not') uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not') +ifneq (,$(findstring MINGW,$(uname_S))) + uname_S := MINGW +endif + ifdef MSVC # avoid the MingW and Cygwin configuration sections uname_S := Windows @@ -588,8 +592,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin SHELL_PATH = /usr/coreutils/bin/bash endif -ifneq (,$(findstring MINGW,$(uname_S))) - uname_S := MINGW +ifeq ($(uname_S),MINGW) pathsep = ; HAVE_ALLOCA_H = YesPlease NO_PREAD = YesPlease ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better 2021-09-28 11:46 ` Ævar Arnfjörð Bjarmason @ 2021-09-28 23:39 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-09-28 23:39 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Carlo Marcelo Arenas Belón, git, t.gummerer, pclouds, jonathantanmy Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In any case, I wonder if we should at least be better off with the > diff-at-the-end on top (untested). > ... > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -11,6 +11,10 @@ uname_R := $(shell sh -c 'uname -r 2>/dev/null || echo not') > uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not') > uname_V := $(shell sh -c 'uname -v 2>/dev/null || echo not') > > +ifneq (,$(findstring MINGW,$(uname_S))) > + uname_S := MINGW > +endif > + It does sound like a better organization to "normalize" different spellings early so that later users of the macro can pretend that there is no "MINGW64_BLA-foo" to worry about. > ifdef MSVC > # avoid the MingW and Cygwin configuration sections > uname_S := Windows > @@ -588,8 +592,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin > SHELL_PATH = /usr/coreutils/bin/bash > endif > -ifneq (,$(findstring MINGW,$(uname_S))) > - uname_S := MINGW > +ifeq ($(uname_S),MINGW) > pathsep = ; > HAVE_ALLOCA_H = YesPlease > NO_PREAD = YesPlease Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS 2021-09-28 9:10 [PATCH 0/3] Makefile: tighten pedantic flag Carlo Marcelo Arenas Belón 2021-09-28 9:10 ` [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better Carlo Marcelo Arenas Belón @ 2021-09-28 9:10 ` Carlo Marcelo Arenas Belón 2021-09-28 9:19 ` Ævar Arnfjörð Bjarmason 2021-09-28 21:19 ` Junio C Hamano 2021-09-28 9:10 ` [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers Carlo Marcelo Arenas Belón 2 siblings, 2 replies; 13+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-28 9:10 UTC (permalink / raw) To: git Cc: t.gummerer, avarab, pclouds, jonathantanmy, Carlo Marcelo Arenas Belón 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to workaround the lack of one from config.mak.autogen. Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and CFLAGS="...", 2019-02-22), that variable is set instead as part of DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so it can be safely removed from config.mak.dev if set instead in the Makefile. This also has the advantage of separating cleanly CFLAGS which are used for building with the ones that provide with diagnostics. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Makefile | 3 ++- config.mak.dev | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 9df565f27b..963b9e7c6b 100644 --- a/Makefile +++ b/Makefile @@ -1200,7 +1200,8 @@ endif # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be # tweaked by config.* below as well as the command-line, both of # which'll override these defaults. -CFLAGS = -g -O2 -Wall +CFLAGS = -g -O2 +DEVELOPER_CFLAGS = -Wall LDFLAGS = CC_LD_DYNPATH = -Wl,-rpath, BASIC_CFLAGS = -I. diff --git a/config.mak.dev b/config.mak.dev index c81be62a5c..90c47d2782 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),) DEVELOPER_CFLAGS += -Werror SPARSE_FLAGS += -Wsparse-error endif -DEVELOPER_CFLAGS += -Wall ifeq ($(filter no-pedantic,$(DEVOPTS)),) DEVELOPER_CFLAGS += -pedantic ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) -- 2.33.0.955.gee03ddbf0e ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS 2021-09-28 9:10 ` [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS Carlo Marcelo Arenas Belón @ 2021-09-28 9:19 ` Ævar Arnfjörð Bjarmason 2021-09-28 11:03 ` Carlo Arenas 2021-09-28 21:19 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-09-28 9:19 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, t.gummerer, pclouds, jonathantanmy On Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote: > 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help > autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to > workaround the lack of one from config.mak.autogen. > > Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and > CFLAGS="...", 2019-02-22), that variable is set instead as part of > DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so > it can be safely removed from config.mak.dev if set instead in the > Makefile. > > This also has the advantage of separating cleanly CFLAGS which are > used for building with the ones that provide with diagnostics. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Makefile | 3 ++- > config.mak.dev | 1 - > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 9df565f27b..963b9e7c6b 100644 > --- a/Makefile > +++ b/Makefile > @@ -1200,7 +1200,8 @@ endif > # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be > # tweaked by config.* below as well as the command-line, both of > # which'll override these defaults. > -CFLAGS = -g -O2 -Wall > +CFLAGS = -g -O2 > +DEVELOPER_CFLAGS = -Wall > LDFLAGS = > CC_LD_DYNPATH = -Wl,-rpath, > BASIC_CFLAGS = -I. > diff --git a/config.mak.dev b/config.mak.dev > index c81be62a5c..90c47d2782 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -Werror > SPARSE_FLAGS += -Wsparse-error > endif > -DEVELOPER_CFLAGS += -Wall > ifeq ($(filter no-pedantic,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -pedantic > ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) This really breaks things for anyone who's relying on specifing CFLAGS now to clobber the default -Wall configuration, e.g. on both xlc and aCC after this: xlc: 1501-289 (W) Option -Wall was incorrectly specified. The option will be ignored. cc: error 1914: bad form for `-W' option I.e. they didn't work before, but I've got CFLAGS="-g -O0" for both in my build scripts, so they didn't get -Wall before, but now they do, so I'll need: CFLAGS=<that> DEVELOPER_CFLAGS= And in my own dev setup I do in config.mak: "CFLAGS=-g -O0", and then rely on config.mak.dev to set -Wall, but if I did e.g.: DEVELOPER= CFLAGS="-g -O0 -Wall" I'd end up with -Wall twice, gcc doesn't complain, but maybe some other toolchains do. For the former case this seems like a really odd and leaky interface since I don't have DEVELOPER=1. Let's leave DEVOPTS, DEVELOPER_CFLAGS etc. etc. only in effect for anyone who turns on that option. Anyway, I don't think it's a no-go to make a change in this direction, and while it would break builds for some perhaps the end result is worth it. I haven't really looked closely enough at the Makefile logic you're untangling to form an opinion on it. But I think this needs to at least have s/DEVELOPER_CFLAGS/WARNING_CFLAGS/g or something. But not to saddle you with an impossible task, wouldn't this whole thing be much easier if we included config.mak* before setting our own CFLAGS etc. defaults? But of course that would break for anyone relying on "+=" working, so I don't know. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS 2021-09-28 9:19 ` Ævar Arnfjörð Bjarmason @ 2021-09-28 11:03 ` Carlo Arenas 0 siblings, 0 replies; 13+ messages in thread From: Carlo Arenas @ 2021-09-28 11:03 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, t.gummerer, pclouds, jonathantanmy On Tue, Sep 28, 2021 at 2:52 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > This really breaks things for anyone who's relying on specifing CFLAGS > now to clobber the default -Wall configuration, e.g. on both xlc and aCC > after this: got it; then it should have been DEVELOPER_CFLAGS+=-Wall the one to keep, and that way there is no need to hack CFLAGS just to disable -Wall in non GNU compilers IMHO of course, that still leaves the question if -Wall is still something we want to have by default for people NOT using DEVELOPER=1 to compile. > I'd end up with -Wall twice, gcc doesn't complain, but maybe some other > toolchains do. clang wouldn't complain either, and indeed you already have 2 -Wall, which is what I was trying to clean up. > But I think this needs to at least have > s/DEVELOPER_CFLAGS/WARNING_CFLAGS/g or something. makes sense, even though I assumed there would be less churn if reusing the variable you came up with. in any case, I think it is clear that this change that I thought will be tiny might be better tackled in a different thread, not to complicate the main reason for this thread, which was to make sure that users that might build the next release (even with DEVELOPER=1) are less likely to find issues than the developers that got their first taste of "pedantic" in master. will wait for more feedback before doing a reroll, but would be nice to get feedback also in the other 2 patches, which hopefully are less problematic. Carlo ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS 2021-09-28 9:10 ` [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS Carlo Marcelo Arenas Belón 2021-09-28 9:19 ` Ævar Arnfjörð Bjarmason @ 2021-09-28 21:19 ` Junio C Hamano 2021-09-28 23:22 ` Carlo Arenas 1 sibling, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2021-09-28 21:19 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón Cc: git, t.gummerer, avarab, pclouds, jonathantanmy Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help > autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to > workaround the lack of one from config.mak.autogen. > > Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and > CFLAGS="...", 2019-02-22), that variable is set instead as part of > DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so > it can be safely removed from config.mak.dev if set instead in the > Makefile. Hmph, don't this break non-developers, though? They now do not get -Wall that they used to? Or am I reading the patch incorrectly? Thanks. > This also has the advantage of separating cleanly CFLAGS which are > used for building with the ones that provide with diagnostics. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > Makefile | 3 ++- > config.mak.dev | 1 - > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 9df565f27b..963b9e7c6b 100644 > --- a/Makefile > +++ b/Makefile > @@ -1200,7 +1200,8 @@ endif > # Set CFLAGS, LDFLAGS and other *FLAGS variables. These might be > # tweaked by config.* below as well as the command-line, both of > # which'll override these defaults. > -CFLAGS = -g -O2 -Wall > +CFLAGS = -g -O2 > +DEVELOPER_CFLAGS = -Wall > LDFLAGS = > CC_LD_DYNPATH = -Wl,-rpath, > BASIC_CFLAGS = -I. > diff --git a/config.mak.dev b/config.mak.dev > index c81be62a5c..90c47d2782 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -6,7 +6,6 @@ ifeq ($(filter no-error,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -Werror > SPARSE_FLAGS += -Wsparse-error > endif > -DEVELOPER_CFLAGS += -Wall > ifeq ($(filter no-pedantic,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -pedantic > ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS 2021-09-28 21:19 ` Junio C Hamano @ 2021-09-28 23:22 ` Carlo Arenas 2021-09-29 4:08 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Carlo Arenas @ 2021-09-28 23:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, t.gummerer, avarab, pclouds, jonathantanmy On Tue, Sep 28, 2021 at 2:19 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > 6163f3f1a4 (config.mak.dev: add -Wall, primarily for -Wformat, to help > > autoconf users, 2018-10-12) adds a second -Wall in config.mak.dev to > > workaround the lack of one from config.mak.autogen. > > > > Since 6d5d4b4e93 (Makefile: allow for combining DEVELOPER=1 and > > CFLAGS="...", 2019-02-22), that variable is set instead as part of > > DEVELOPER_FLAGS which won't be overriden by config.mak.autogen, so > > it can be safely removed from config.mak.dev if set instead in the > > Makefile. > > Hmph, don't this break non-developers, though? > > They now do not get -Wall that they used to? Or am I reading the > patch incorrectly? It is subtle and apparently problematic as some users[1] rely on removing -Wall to support non GNU compilers, but my change kept the -Wall by default by making sure it got set in DEVELOPER_FLAGS instead (which as Ævar points out might be a layering violation and hence might need more planning). Either way I'll be dropping this series for now, to make sure that the minimal change is put forward as part of js/win-lazyload-buildfix instead. Carlo [1] https://lore.kernel.org/git/CAPUEspj_hOXRc2d+c+DTvShgWX8NH+fXKD4Pk+_G=nj9Z97VnQ@mail.gmail.com/ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS 2021-09-28 23:22 ` Carlo Arenas @ 2021-09-29 4:08 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-09-29 4:08 UTC (permalink / raw) To: Carlo Arenas; +Cc: git, t.gummerer, avarab, pclouds, jonathantanmy Carlo Arenas <carenas@gmail.com> writes: > Either way I'll be dropping this series for now, to make sure that the > minimal change is put forward as part of js/win-lazyload-buildfix > instead. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers 2021-09-28 9:10 [PATCH 0/3] Makefile: tighten pedantic flag Carlo Marcelo Arenas Belón 2021-09-28 9:10 ` [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better Carlo Marcelo Arenas Belón 2021-09-28 9:10 ` [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS Carlo Marcelo Arenas Belón @ 2021-09-28 9:10 ` Carlo Marcelo Arenas Belón 2021-09-28 12:07 ` Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 13+ messages in thread From: Carlo Marcelo Arenas Belón @ 2021-09-28 9:10 UTC (permalink / raw) To: git Cc: t.gummerer, avarab, pclouds, jonathantanmy, Carlo Marcelo Arenas Belón 1da1580e4c (Makefile: detect compiler and enable more warnings in DEVELOPER=1, 2018-04-14), includes an $(or) of two different filters to check for both gcc and clang versions. As shown in a previous patch, a simpler syntax is available so apply the same logic here also for consistency. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- config.mak.dev | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.mak.dev b/config.mak.dev index 90c47d2782..b66fae8665 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -31,7 +31,7 @@ ifneq ($(filter clang4,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare endif -ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) +ifneq ($(filter clang4 gcc6,$(COMPILER_FEATURES)),) DEVELOPER_CFLAGS += -Wextra # if a function is public, there should be a prototype and the right # header file should be included. If not, it should be static. -- 2.33.0.955.gee03ddbf0e ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers 2021-09-28 9:10 ` [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers Carlo Marcelo Arenas Belón @ 2021-09-28 12:07 ` Ævar Arnfjörð Bjarmason 2021-09-28 21:30 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-09-28 12:07 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, t.gummerer, pclouds, jonathantanmy On Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote: > 1da1580e4c (Makefile: detect compiler and enable more warnings in > DEVELOPER=1, 2018-04-14), includes an $(or) of two different filters > to check for both gcc and clang versions. > > As shown in a previous patch, a simpler syntax is available so apply > the same logic here also for consistency. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > config.mak.dev | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/config.mak.dev b/config.mak.dev > index 90c47d2782..b66fae8665 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -31,7 +31,7 @@ ifneq ($(filter clang4,$(COMPILER_FEATURES)),) > DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare > endif > > -ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),) > +ifneq ($(filter clang4 gcc6,$(COMPILER_FEATURES)),) > DEVELOPER_CFLAGS += -Wextra > # if a function is public, there should be a prototype and the right > # header file should be included. If not, it should be static. This looks like a good cleanup and ends up being much more readable. I wonder if eventually a larger change to simplify this like perhaps the below wouldn't be nicer. I didn't test it carefully & may have gotten the logic wrong, which I think somewhat makes the point that reading this ifeq/ifneq logic (especially the nested bit at the end) is a bit hard, at least for me:) Anyway, feel free to ignore the below, and I think it's certainly not needed for this series, just my 0.02 if you're eventually refactoring some of this. diff --git a/config.mak.dev b/config.mak.dev index 0a87d8cbe9d..df27340b4b0 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -2,6 +2,14 @@ ifndef COMPILER_FEATURES COMPILER_FEATURES := $(shell ./detect-compiler $(CC)) endif +# These are all the empty string if the compiler *isn't* X or +# earlier. Note that clang v5, v6 etc. also qualify as "have v4". +CC_HAVE_CLANG4 = $(filter clang4,$(COMPILER_FEATURES)) +CC_HAVE_GCC4 = $(filter gcc4,$(COMPILER_FEATURES)) +CC_HAVE_GCC5 = $(filter gcc5,$(COMPILER_FEATURES)) +CC_HAVE_GCC6 = $(filter gcc6,$(COMPILER_FEATURES)) +CC_HAVE_GCC10 = $(filter gcc10,$(COMPILER_FEATURES)) + ifeq ($(filter no-error,$(DEVOPTS)),) DEVELOPER_CFLAGS += -Werror SPARSE_FLAGS += -Wsparse-error @@ -9,9 +17,9 @@ endif DEVELOPER_CFLAGS += -Wall ifeq ($(filter no-pedantic,$(DEVOPTS)),) DEVELOPER_CFLAGS += -pedantic -ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) +ifneq ($(CC_HAVE_CLANG4)$(CC_HAVE_GCC5),) DEVELOPER_CFLAGS += -Wpedantic -ifneq ($(filter gcc10,$(COMPILER_FEATURES)),) +ifneq ($(CC_HAVE_GCC10),) ifeq ($(uname_S),MINGW) DEVELOPER_CFLAGS += -Wno-pedantic-ms-format DEVELOPER_CFLAGS += -Wno-incompatible-pointer-types @@ -29,11 +37,11 @@ DEVELOPER_CFLAGS += -Wunused DEVELOPER_CFLAGS += -Wvla DEVELOPER_CFLAGS += -fno-common -ifneq ($(filter clang4,$(COMPILER_FEATURES)),) +ifneq ($(CC_HAVE_CLANG4),) DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare endif -ifneq ($(filter clang4 gcc6,$(COMPILER_FEATURES)),) +ifneq ($(CC_HAVE_CLANG4)$(CC_HAVE_GCC6),) DEVELOPER_CFLAGS += -Wextra # if a function is public, there should be a prototype and the right # header file should be included. If not, it should be static. @@ -49,8 +57,8 @@ endif # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c # not worth fixing since newer compilers correctly stop complaining -ifneq ($(filter gcc4,$(COMPILER_FEATURES)),) -ifeq ($(filter gcc5,$(COMPILER_FEATURES)),) +ifneq ($(CC_HAVE_GCC4),) +ifeq ($(CC_HAVE_GCC5),) DEVELOPER_CFLAGS += -Wno-uninitialized endif endif ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers 2021-09-28 12:07 ` Ævar Arnfjörð Bjarmason @ 2021-09-28 21:30 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-09-28 21:30 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Carlo Marcelo Arenas Belón, git, t.gummerer, pclouds, jonathantanmy Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Anyway, feel free to ignore the below, and I think it's certainly not > needed for this series, just my 0.02 if you're eventually refactoring > some of this. > > diff --git a/config.mak.dev b/config.mak.dev > index 0a87d8cbe9d..df27340b4b0 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -2,6 +2,14 @@ ifndef COMPILER_FEATURES > COMPILER_FEATURES := $(shell ./detect-compiler $(CC)) > endif > > +# These are all the empty string if the compiler *isn't* X or > +# earlier. Note that clang v5, v6 etc. also qualify as "have v4". I had to read this three times and still cannot decide what it wants ot say. > +CC_HAVE_CLANG4 = $(filter clang4,$(COMPILER_FEATURES)) > +CC_HAVE_GCC4 = $(filter gcc4,$(COMPILER_FEATURES)) > +CC_HAVE_GCC5 = $(filter gcc5,$(COMPILER_FEATURES)) > +CC_HAVE_GCC6 = $(filter gcc6,$(COMPILER_FEATURES)) > +CC_HAVE_GCC10 = $(filter gcc10,$(COMPILER_FEATURES)) It is empty if the compiler isn't X. It is also empty if the compiler isn't earlier than X? That would mean version (X+1) would qualify, as it is not X, which is not what we want. I think you meant "... empty string, unless the compiler is X or later." Also, I often see "Note that" to imply "despite what I just said", but as far as I can tell CLANG4 is not all that special and follows the same general rule. Perhaps "Note that" -> "For example," is needed to clarify. Having said all that, I find that the original ifneq ($(filter x y, $(COMPILER_FEATURES)),) idiom is readable enough, and ifneq ($(CC_HAVE_X)$(CC_HAVE_Y),) does not necessarily make it easier to spot X and Y that are being checked with the conditional. > ifeq ($(filter no-error,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -Werror > SPARSE_FLAGS += -Wsparse-error > @@ -9,9 +17,9 @@ endif > DEVELOPER_CFLAGS += -Wall > ifeq ($(filter no-pedantic,$(DEVOPTS)),) > DEVELOPER_CFLAGS += -pedantic > -ifneq ($(filter clang4 gcc5,$(COMPILER_FEATURES)),) > +ifneq ($(CC_HAVE_CLANG4)$(CC_HAVE_GCC5),) ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-29 4:08 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-28 9:10 [PATCH 0/3] Makefile: tighten pedantic flag Carlo Marcelo Arenas Belón 2021-09-28 9:10 ` [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better Carlo Marcelo Arenas Belón 2021-09-28 11:46 ` Ævar Arnfjörð Bjarmason 2021-09-28 23:39 ` Junio C Hamano 2021-09-28 9:10 ` [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS Carlo Marcelo Arenas Belón 2021-09-28 9:19 ` Ævar Arnfjörð Bjarmason 2021-09-28 11:03 ` Carlo Arenas 2021-09-28 21:19 ` Junio C Hamano 2021-09-28 23:22 ` Carlo Arenas 2021-09-29 4:08 ` Junio C Hamano 2021-09-28 9:10 ` [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers Carlo Marcelo Arenas Belón 2021-09-28 12:07 ` Ævar Arnfjörð Bjarmason 2021-09-28 21:30 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.