patch, 3 partsMakefile: 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

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

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.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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.

Thanks.

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

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 <avarab@gmail.com> wrote:

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.

clang wouldn't complain either, and indeed you already have 2 -Wall, which is what I was trying to clean up.

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

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

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.

On Tue, Sep 28, 2021 at 2:19 PM Junio C Hamano <gitster@pobox.com> wrote:

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/

Carlo Arenas <carenas@gmail.com> writes:

Thanks.

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

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.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

I had to read this three times and still cannot decide what it wants ot say.

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.