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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

> 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.