[PATCH 0/3] Makefile: tighten pedantic flag

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

On Tue, Sep 28 2021, Carlo Marcelo Arenas Belón 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:

    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.

On Tue, Sep 28, 2021 at 2:52 AM Ævar Arnfjörð Bjarmason
[off-list ref] 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

On Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote:

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

On Tue, Sep 28 2021, Carlo Marcelo Arenas Belón wrote:

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