| Message ID | 20210928091054.78895-4-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: tighten pedantic flag | expand |
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
Æ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),)
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.
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(-)