Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces

From: Jeff King <peff@peff.net>
Date: Jul 14, 2022, 21:54 UTC
Message-ID: <YtCQl1oinrVnfa+6@coredump.intra.peff.net>
In-Reply-To: <220712.864jzm65mk.gmgdl@evledraar.gmail.com>

On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 6 quoted lines

> >> I'm in favor of this. It would, of course, require extra
> >> special-casing for Apple's clang for which the version number bears no
> >> resemblance to reality since Apple invents their own version numbers.
> 
> FWIW I was imagining just providing that -Wno-* on clang versions <= 11,
> not special-casing Apple's in particular.

It's not about special-casing Apple in particular. It's that our detect-compiler script does not understand which version of clang is used for Apple's compiler. Their version numbers are totally mismatched.

So you either have to say "turn off this warning for clang totally", or do the wrong thing when Apple's compiler is in use.

> I have a local patches that carry forward the idea I had in that thread,
> i.e. to drop all this version detection insanity and just compile a C
> program to detect the compiler.

Hmm. I prefer your earlier suggestion to use "$(CC) -E". This tool has to run on every invocation of "make", so the lighter-weight it is, the better. You say later...

> The compilation is then triggered by the include in config.mak.dev,
> which has a corresponding rule that creates the C program, then the
> generated *.mak, so once we do it once we're only ever including an
> already generated text file.

but that implies we don't have a dependency on the compiler itself. You'd want to at least depend on the name of the compiler. But that would also miss if running "clang" changes which version of clang you're running. I know upgrading your compiler is rare-ish, but this is exactly the kind of thing I expect to bite at the most annoying time (when you're switching around versions to try to figure out how they behave).

> 	#ifdef __clang__
> 	#if __clang_major__ >= 7
> 		fn(util, "NEEDS_std-eq-gnu99", "1");
> 	#endif

This is still gross version detection, but I don't think we can avoid it. However...

> 	#if __has_warning("-Wextra")
> 		fn(util, "HAS_Wextra", "1");
> 	#endif

...this is much nicer. It could still be implemented purely via "-E", as far as I can see, like:

  #if __has_warning("-Wextra")
  HAS_Wextra = 1
  #endif

But then we end up having to do version comparisons for gcc anyway:

> 	#if __GNUC__ >= 6
> 		fn(util, "NEEDS_std-eq-gnu99", "1");
> 		fn(util, "HAS_Wextra", "1");

so it feels like we're back where we started. You've just encoded the version checks in a different spot.

I dunno. I don't find this significantly less gross than the status quo. I don't mind getting the version via "-E" rather than "-v", but whether the policy logic is in cpp, or in shell, or in the Makefile, it still needs to exist. Putting it in cpp allows using has_warning(), but since that isn't available everywhere, I'm not sure it buys us much.

-Peff