threads / patch / 49898

patchconfig.mak.dev: enable -Wpedantic in clang

Subject: [PATCH] config.mak.dev: enable -Wpedantic in clang

## tl;dr

5 messages between Nov 27, 2018 and Nov 29, 2018. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Carlo Marcelo Arenas Belón· Nov 27, 2018, 10:05 UTC · lore

DEVOPTS=pedantic adds -pedantic to the compiler flags, but misses on some diagnostics when using clang, and that are only enabled with -Wpedantic

46c0eb5843 ("files-backend.c: fix build error on Solaris", 2018-11-25) fixes an issue that was visible also with gcc but not clang so correct that with the hope that in the future CI could be used for early detection of similar issues

-Wpedantic is only enabled for clang 10 or higher (only available in macOS with latest Xcode) but this restriction should be relaxed further as more environments are tested

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 config.mak.dev | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Show changes to config.mak.dev +7 −4

diff --git a/config.mak.dev b/config.mak.dev
index bbeeff44fe..ad25beacd8 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,8 +1,15 @@
+ifndef COMPILER_FEATURES
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+endif
+
 ifeq ($(filter no-error,$(DEVOPTS)),)
 CFLAGS += -Werror
 endif
 ifneq ($(filter pedantic,$(DEVOPTS)),)
 CFLAGS += -pedantic
+ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
+CFLAGS += -Wpedantic
+endif
 # don't warn for each N_ use
 CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0
 endif
@@ -16,10 +23,6 @@ CFLAGS += -Wstrict-prototypes
 CFLAGS += -Wunused
 CFLAGS += -Wvla
 
-ifndef COMPILER_FEATURES
-COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
-endif
-
 ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
 CFLAGS += -Wtautological-constant-out-of-range-compare
 endif

-- 
2.20.0.rc1

Eric Sunshine· Nov 27, 2018, 10:53 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH] config.mak.dev: enable -Wpedantic in clang

On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

> [...]
> -Wpedantic is only enabled for clang 10 or higher (only available in macOS
> with latest Xcode) but this restriction should be relaxed further as more
> environments are tested

We know from [1] that the clang version number "10" is an Apple fiction/invention. Perhaps this commit message can be worded a bit more carefully to avoid confusing readers who are not clued in to that fact.

[1]: https://public-inbox.org/git/CAPig+cRQXQ_DowS2Dsc1x3TAGJjnWig7P4eYS4kQ+C2piAdSWA@mail.gmail.com/

Show 5 quoted lines

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
> +CFLAGS += -Wpedantic
> +endif

Should this condition be tightened to match only for OSX since there is no such clang version number in the rest world outside of Apple?

Carlo Arenas· Nov 27, 2018, 15:48 UTC · re: Eric Sunshine · lore

Re: [PATCH] config.mak.dev: enable -Wpedantic in clang

On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 7 quoted lines

> On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón
> > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
> > +CFLAGS += -Wpedantic
> > +endif
>
> Should this condition be tightened to match only for OSX since there
> is no such clang version number in the rest world outside of Apple?

instead, I changed it to clang4 and tested it in a FreeBSD 11 box I was able to get a hold off, would that work better? will update patch accordingly then

Carlo

Eric Sunshine· Nov 27, 2018, 19:03 UTC · re: Carlo Arenas · lore

Re: [PATCH] config.mak.dev: enable -Wpedantic in clang

On Tue, Nov 27, 2018 at 10:48 AM Carlo Arenas <carenas@gmail.com> wrote:

Show 12 quoted lines

> On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón
> > > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),)
> > > +CFLAGS += -Wpedantic
> > > +endif
> >
> > Should this condition be tightened to match only for OSX since there
> > is no such clang version number in the rest world outside of Apple?
>
> instead, I changed it to clang4 and tested it in a FreeBSD 11 box I
> was able to get a hold off, would that work better?
> will update patch accordingly then

Playing Devi's Advocate, what if Apple's clang "8" was, in reality, real-world clang 3? Then this condition would incorrectly enable the compiler option on Apple for a (real) clang version below 4. For this reason, it seems we shouldn't be trusting only the clang version number when dealing with Apple.

(I suspect that it won't make a difference in actual practice, so it may be reasonable to punt on this issue until/if someone complains.)

Junio C Hamano· Nov 29, 2018, 05:13 UTC · re: Eric Sunshine · lore

Re: [PATCH] config.mak.dev: enable -Wpedantic in clang

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 8 quoted lines

> Playing Devi's Advocate, what if Apple's clang "8" was, in reality,
> real-world clang 3? Then this condition would incorrectly enable the
> compiler option on Apple for a (real) clang version below 4. For this
> reason, it seems we shouldn't be trusting only the clang version
> number when dealing with Apple.
>
> (I suspect that it won't make a difference in actual practice, so it
> may be reasonable to punt on this issue until/if someone complains.)

Why do we care which true version of clang is being used here in the first place? Is it because some version of clang (take -Wpedantic but misbehave | barf when -Wpedantic is given) while some others are OK?

If the only problem is that some version of clang barf when the option is given, perhaps we can do a trial-compile of helloworld.c or something, or is that something we are trying to avoid in the first place?

It appears to me that ./detect-compiler tool (by the way, perhaps we should start moving these build-helper-tools sitting at the top level down to ./build-helpers/ subdirectory or something so that we can focus on the source code when running "ls" at the top level of the hierarchy) can become more intelligent to help things like this.