Re: [PATCH 3/3] compat/bswap.h: Detect endianness from XL C compiler macros

From: David Michael <fedora.dm0@gmail.com>
Date: Oct 26, 2014, 20:04 UTC
Message-ID: <CAEvUa7mcJhYwivQjbWnVEXj_+fhCS+2edCFWjmEotOXKahQdww@mail.gmail.com>
In-Reply-To: <20141026183815.GB18144@peff.net>

On Sun, Oct 26, 2014 at 2:38 PM, Jeff King <peff@peff.net> wrote:

Show 37 quoted lines

> On Sun, Oct 26, 2014 at 01:34:26PM -0400, David Michael wrote:
>
>> diff --git a/compat/bswap.h b/compat/bswap.h
>> index f6fd9a6..7fed637 100644
>> --- a/compat/bswap.h
>> +++ b/compat/bswap.h
>> @@ -122,6 +122,10 @@ static inline uint64_t git_bswap64(uint64_t x)
>>  #  define GIT_BYTE_ORDER GIT_BIG_ENDIAN
>>  # elif defined(_LITTLE_ENDIAN) && !defined(_BIG_ENDIAN)
>>  #  define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
>> +# elif defined(__THW_BIG_ENDIAN__) && !defined(__THW_LITTLE_ENDIAN__)
>> +#  define GIT_BYTE_ORDER GIT_BIG_ENDIAN
>> +# elif defined(__THW_LITTLE_ENDIAN__) && !defined(__THW_BIG_ENDIAN__)
>> +#  define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
>
> Out of curiosity, is there ever a case where _both_ are defined? That
> is, would:
>
> diff --git a/compat/bswap.h b/compat/bswap.h
> index f6fd9a6..b78a0bd 100644
> --- a/compat/bswap.h
> +++ b/compat/bswap.h
> @@ -122,6 +122,10 @@ static inline uint64_t git_bswap64(uint64_t x)
>  #  define GIT_BYTE_ORDER GIT_BIG_ENDIAN
>  # elif defined(_LITTLE_ENDIAN) && !defined(_BIG_ENDIAN)
>  #  define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
> +# elif defined(__THW_BIG_ENDIAN__)
> +#  define GIT_BYTE_ORDER GIT_BIG_ENDIAN
> +# elif defined(__THW_LITTLE_ENDIAN__)
> +#  define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
>  # else
>  #  error "Cannot determine endianness"
>  # endif
>
> be enough, or is that used to mark some other special case? Even if not,
> there is not much downside to the more thorough conditions you put
> above, but as a reviewer I wondered if there is something else going on.

I'm not 100% sure if __THW_LITTLE_ENDIAN__ will ever be defined, so I'd be okay with dropping those references completely. There is a recent version of the compiler for little endian Linux distributions, but I haven't found the documentation for it. (The product documentation still seems to only refer to the big endian Linux version.) The compiler's macro may be redundant in this case anyway, since Linux systems should have <bits/endian.h> supplying that information.

I only used both macros for completeness; the __THW_BIG_ENDIAN__ macro (defined to 1 on z/OS and AIX) is what I actually needed here. z/OS doesn't seem to have any other compile-time byte order indicator, short of testing for the OS itself.

Would you prefer the two-line patch to only test for the big endian macro, or maybe just test for __MVS__ to look at the OS?

Thanks.

David