Re: [PATCH] unpack-objects: fix compilation warning/error due to missing braces
On Thu, Jul 14 2022, Jeff King wrote:
Show 15 quoted lines
> On Tue, Jul 12, 2022 at 11:16:10AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> >> 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 missed the part where we don't even detect the version in that case, but anyway, just turning it off on clang && OSX seems fine. This is -Wmissing-braces, we'll catch it elsewhere (CI etc.)
Show 7 quoted lines
>> 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...
We only compile & run the binary once, then save the result to a *.mak file, the intention is to get rid of running these binaries or script on every single invocation.
Show 11 quoted lines
>> 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).
That's true, I considered that OK, and still do. I.e. we use this for config.mak, where we are opting you in to never flags.
If your compiler name changes your GIT-BUILD-OPTIONS will change, so we'll re-build and re-detect.
But yes, if your CC=gcc and you change gcc from under us we won't re-detect and re-build.
But isn't that fine? You just won't get newer flags, you might downgrade your gcc and get an error, but generally speaking make-based systems really don't try to detect "anything on the OS" changed.
Still, I could easily add something where we one-off run "command -v $(CC)", save that to a file, and then add that to our "make" dependency tree.
So then if the compiler binary's mtime changes we'd re-build, and we still wouldn't run something on every invocation.
It just seems to be quite pedantic about correctness, is all :)
Show 7 quoted lines
>> #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...
Yes, this part isn't really nicer, although with this method we could also avoid this sort of thing by just trying the flag out & caching that, but this seemed OK.
Show 10 quoted lines
>> #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*nod*, I wish GCC had that.
Show 8 quoted lines
> 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.
We're really not, the reason I did this was because i tried to add support for xlc and suncc to this script, we don't even parse detect-compiler on Solaris now, as its /bin/sh isn't compatible with it. So to begin with we'd need to hoist the "shell detection and script generation" out ...
Show 5 quoted lines
> 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.
Compilers universally support "who and what am I?" via their native macros, but we're trying to do it the really hard way via parsing version output.
Eric Sunshine
