[PATCH 0/3] Makefile: tighten pedantic flag

[PATCH 0/3] Makefile: tighten pedantic flag

[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

[PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

[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

[PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS

[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

[PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers

[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

Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS

[Carlo Arenas]

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

Re: [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers

[Ævar Arnfjörð Bjarmason]

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

Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS

[Junio C Hamano]

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)),)

Re: [PATCH 3/3] config.mak.dev: simplify compiler check for multiple compilers

[Junio C Hamano]

Æ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),)

Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS

[Carlo Arenas]

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/

Re: [PATCH 1/3] Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format better

[Junio C Hamano]

Æ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.

Re: [PATCH 2/3] Makefile: avoid multiple -Wall in CFLAGS

[Junio C Hamano]

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.