[PATCH] config.mak.dev: fix typo when enabling -Wpedantic

[PATCH] config.mak.dev: fix typo when enabling -Wpedantic

[Taylor Blau]

In ebd2e4a13a (Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format
better, 2021-09-28), we tightened our Makefile's behavior to only enable
-Wpedantic when compiling with either gcc5/clang4 or greater as older
compiler versions did not have support for -Wpedantic.

Commit ebd2e4a13a was looking for either "gcc5" or "clang4" to appear in
the COMPILER_FEATURES variable, combining the two "$(filter ...)"
searches with an "$(or ...)".

But ebd2e4a13a has a typo where instead of writing:

    ifneq ($(or ($filter ...),$(filter ...)),)

we wrote:

    ifneq (($or ($filter ...),$(filter ...)),)

Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently
recent compiler version) to barf:

    $ make DEVELOPER=1
    config.mak.dev:13: extraneous text after 'ifneq' directive
    [...]

Correctly combine the results of the two "$(filter ...)" operations by
using "$(or ...)", not "$or".

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.mak.dev | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/config.mak.dev b/config.mak.dev
index 1ce4c70613..5229c35484 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -10,7 +10,7 @@ endif
 DEVELOPER_CFLAGS += -Wall
 ifeq ($(filter no-pedantic,$(DEVOPTS)),)
 DEVELOPER_CFLAGS += -pedantic
-ifneq (($or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
+ifneq ($(or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
 DEVELOPER_CFLAGS += -Wpedantic
 ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)
 ifeq ($(uname_S),MINGW)
-- 
2.45.2.705.gad6bdba207.dirty

Re: [PATCH] config.mak.dev: fix typo when enabling -Wpedantic

[Elijah Newren]

On Fri, Jul 5, 2024 at 11:51 AM Taylor Blau <me@ttaylorr.com> wrote:
>
> In ebd2e4a13a (Makefile: restrict -Wpedantic and -Wno-pedantic-ms-format
> better, 2021-09-28), we tightened our Makefile's behavior to only enable
> -Wpedantic when compiling with either gcc5/clang4 or greater as older
> compiler versions did not have support for -Wpedantic.
>
> Commit ebd2e4a13a was looking for either "gcc5" or "clang4" to appear in
> the COMPILER_FEATURES variable, combining the two "$(filter ...)"
> searches with an "$(or ...)".
>
> But ebd2e4a13a has a typo where instead of writing:
>
>     ifneq ($(or ($filter ...),$(filter ...)),)
>
> we wrote:
>
>     ifneq (($or ($filter ...),$(filter ...)),)
>
> Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently
> recent compiler version) to barf:
>
>     $ make DEVELOPER=1
>     config.mak.dev:13: extraneous text after 'ifneq' directive
>     [...]
>
> Correctly combine the results of the two "$(filter ...)" operations by
> using "$(or ...)", not "$or".
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  config.mak.dev | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index 1ce4c70613..5229c35484 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -10,7 +10,7 @@ endif
>  DEVELOPER_CFLAGS += -Wall
>  ifeq ($(filter no-pedantic,$(DEVOPTS)),)
>  DEVELOPER_CFLAGS += -pedantic
> -ifneq (($or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
> +ifneq ($(or $(filter gcc5,$(COMPILER_FEATURES)),$(filter clang4,$(COMPILER_FEATURES))),)
>  DEVELOPER_CFLAGS += -Wpedantic
>  ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)
>  ifeq ($(uname_S),MINGW)
> --
> 2.45.2.705.gad6bdba207.dirty

Best viewed using `--color-words=.`, so you can see that it's just one
parenthesis that is moving, and in particular just moves to the right
one spot.  (Which you already called out nicely in the commit
message.)  Anyway, looks good to me.

Re: [PATCH] config.mak.dev: fix typo when enabling -Wpedantic

[Jeff King]

On Fri, Jul 05, 2024 at 02:51:09PM -0400, Taylor Blau wrote:

> But ebd2e4a13a has a typo where instead of writing:
> 
>     ifneq ($(or ($filter ...),$(filter ...)),)
> 
> we wrote:
> 
>     ifneq (($or ($filter ...),$(filter ...)),)

Good catch. Your fix is obviously the right thing. But one thing that
puzzled me...

> Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently
> recent compiler version) to barf:
> 
>     $ make DEVELOPER=1
>     config.mak.dev:13: extraneous text after 'ifneq' directive
>     [...]
> 
> Correctly combine the results of the two "$(filter ...)" operations by
> using "$(or ...)", not "$or".

...why don't I see this error? Based on the bug, I think that we'll
always pass -Wpedantic, even for old compilers (because our weird "or"
will never be the empty string).

So I could understand if the symptom was then that when you have an old
compiler, we feed it -Wpedantic and it complains (though the fact that
nobody noticed such a behavior makes me wonder if we even care about
such old compilers now?).

But why does make complain here only sometimes? Does it depend on the
version of make?

-Peff

Re: [PATCH] config.mak.dev: fix typo when enabling -Wpedantic

[Taylor Blau]

On Sat, Jul 06, 2024 at 02:31:43AM -0400, Jeff King wrote:
> > Causing our Makefile (when invoked with DEVELOPER=1, and a sufficiently
> > recent compiler version) to barf:
> >
> >     $ make DEVELOPER=1
> >     config.mak.dev:13: extraneous text after 'ifneq' directive
> >     [...]
> >
> > Correctly combine the results of the two "$(filter ...)" operations by
> > using "$(or ...)", not "$or".
>
> ...why don't I see this error? Based on the bug, I think that we'll
> always pass -Wpedantic, even for old compilers (because our weird "or"
> will never be the empty string).
>
> So I could understand if the symptom was then that when you have an old
> compiler, we feed it -Wpedantic and it complains (though the fact that
> nobody noticed such a behavior makes me wonder if we even care about
> such old compilers now?).
>
> But why does make complain here only sometimes? Does it depend on the
> version of make?

It seems to depend on the version of make you're using. On my system,
'make' is GNU Make 4.4.90, which has the more restrictive checks around
the recipe prefix in nested conditionals.

With that version (and the pre-image of this commit), I get:

    $ make -v | head -1 && make DEVELOPER=1 2>&1 | head -1
    GNU Make 4.4.90
    config.mak.dev:13: extraneous text after 'ifneq' directive

, but with /usr/bin/make (which on my machine is GNU Make 4.3), I
instead get:

    $ /usr/bin/make -v | head -1 && /usr/bin/make DEVELOPER=1 2>&1 | head -1
    GNU Make 4.3
    GIT_VERSION = 2.45.2.746.g06e570c0df

I think other factors that might be at play here are (a) whether or not
you have DEVOPTS=no-pedantic (in which case you'd bypass this entire
part of config.mak.dev), and (b) whether or not you have a sufficiently
recent compiler.

It is tempting to just want to rip out support for older compilers, but
given that ebd2e4a13a (Makefile: restrict -Wpedantic and
-Wno-pedantic-ms-format better, 2021-09-28) is only three years old, I
imagine that some builders may still want support for older / pre-GCC 4
compilers.

Thanks,
Taylor

Re: [PATCH] config.mak.dev: fix typo when enabling -Wpedantic

[Taylor Blau]

On Sat, Jul 06, 2024 at 11:15:43AM -0400, Taylor Blau wrote:
> It is tempting to just want to rip out support for older compilers, but
> given that ebd2e4a13a (Makefile: restrict -Wpedantic and
> -Wno-pedantic-ms-format better, 2021-09-28) is only three years old, I
> imagine that some builders may still want support for older / pre-GCC 4
> compilers.

Hmm... thinking on it more, edb2e4a13a hasn't been working at all on the
older versions of Make that people with ancient compilers are likely
also using. So it's possible that that commit isn't doing as much as we
think, in which case we could rip it out altogether.

I don't think you can actually get rid of the detect-compiler script,
since we do have filters for more recent compilers (e.g., "gcc10", and
so on). But it would be a step in the right direction :-).

Thanks,
Taylor

Re: [PATCH] config.mak.dev: fix typo when enabling -Wpedantic

[Jeff King]

On Sat, Jul 06, 2024 at 11:15:43AM -0400, Taylor Blau wrote:

> > But why does make complain here only sometimes? Does it depend on the
> > version of make?
> 
> It seems to depend on the version of make you're using. On my system,
> 'make' is GNU Make 4.4.90, which has the more restrictive checks around
> the recipe prefix in nested conditionals.
> 
> With that version (and the pre-image of this commit), I get:
> 
>     $ make -v | head -1 && make DEVELOPER=1 2>&1 | head -1
>     GNU Make 4.4.90
>     config.mak.dev:13: extraneous text after 'ifneq' directive
> 
> , but with /usr/bin/make (which on my machine is GNU Make 4.3), I
> instead get:
> 
>     $ /usr/bin/make -v | head -1 && /usr/bin/make DEVELOPER=1 2>&1 | head -1
>     GNU Make 4.3
>     GIT_VERSION = 2.45.2.746.g06e570c0df

Ah, thanks for digging. That makes perfect sense (I'm on 4.3 from Debian
unstable).

> On Sat, Jul 06, 2024 at 11:15:43AM -0400, Taylor Blau wrote:
> > It is tempting to just want to rip out support for older compilers, but
> > given that ebd2e4a13a (Makefile: restrict -Wpedantic and
> > -Wno-pedantic-ms-format better, 2021-09-28) is only three years old, I
> > imagine that some builders may still want support for older / pre-GCC 4
> > compilers.
> 
> Hmm... thinking on it more, edb2e4a13a hasn't been working at all on the
> older versions of Make that people with ancient compilers are likely
> also using. So it's possible that that commit isn't doing as much as we
> think, in which case we could rip it out altogether.

Right, that's what I meant by "nobody seems to have complained". But as
you note, it's not like it gets rid of the detect-compiler script. It's
just this one check. So it doesn't hurt much to leave it.

-Peff