Re: [PATCH 1/3] sha1dc: update from my PR #36

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 27, 2017, 18:07 UTC
Message-ID: <87y3sdjocr.fsf@gmail.com>
In-Reply-To: <xmqqinjh2zqu.fsf@gitster.mtv.corp.google.com>

On Tue, Jun 27 2017, Junio C. Hamano jotted:

Show 27 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> +#else /* Not under GCC-alike or glibc or <processor whitelist>  or <processor blacklist> */
>>> +
>>> +#ifdef _BIG_ENDIAN
>>> +/*
>>> + * Solaris / illumos defines either _LITTLE_ENDIAN or _BIG_ENDIAN in
>>> + * <sys/isa_defs.h>.
>>> + */
>>> +#define SHA1DC_BIGENDIAN
>>
>> This makes readers of this patch wonder why we assume platforms
>> won't define _LITTLE_ENDIAN and _BIG_ENDIAN at the same time, just
>> like we saw in the section with __BIG_ENDIAN above.
>
> To be a bit more constructive, I'd feel it MUCH safer, if this "If
> _BIG_ENDIAN is defined, set SHA1DC_BIGENDIAN" is done _ONLY_ when
> we definitively KNOW that we are on Solaris, something like:
>
> 	#if defined(__sun) && defined(_BIG_ENDIAN)
> 	/*
> 	 * Solaris ...
> 	 */
> 	#define SHA1DC_BIGENDIAN
> 	#endif

Yes, this would be better, but I don't know what macro test to use to test for Solaris. Oracle documents defined(sun), but that doesn't work on the Solaris versions we tested, and looking/searching illumos headers I didn't find anything obvious.

The __sun define is ours, and would work for us, but upstream couldn't take it.

>> Thanks, but this is starting to feel like watching a whack-a-mole
>> played while blindfolded.  At some point, somebody upstream should
>> declare that enough is enough and introduce the "SHA1DC_FORCE_ENDIAN"
>> macro.