Thread (9 messages) 9 messages, 4 authors, 2022-07-15

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

From: Jeff King <hidden>
Date: 2022-07-14 21:54:36

Possibly related (same subject, not in this thread)

On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help