patchAdd extra logic required to detect endianness on Solaris

>  #if !defined(__BYTE_ORDER)
> +/* Known to be needed on Solaris but designed to potentially more portable */
> +
> +#if !defined(__BIG_ENDIAN)
> +#define __BIG_ENDIAN 4321
> +#endif
> +
> +#if !defined(__LITTLE_ENDIAN)
> +#define __LITTLE_ENDIAN 1234
> +#endif
> +
> +#if defined(_BIG_ENDIAN)
> +#define __BYTE_ORDER __BIG_ENDIAN
> +#endif
> +#if defined(_LITTLE_ENDIAN)
> +#define __BYTE_ORDER __LITTLE_ENDIAN
> +#endif

> Charles Bailey <cbailey32@bloomberg.net> writes:
>
>>  #if !defined(__BYTE_ORDER)
>> +/* Known to be needed on Solaris but designed to potentially more portable */
>> +
>> +#if !defined(__BIG_ENDIAN)
>> +#define __BIG_ENDIAN 4321
>> +#endif
>> +
>> +#if !defined(__LITTLE_ENDIAN)
>> +#define __LITTLE_ENDIAN 1234
>> +#endif
>> +
>> +#if defined(_BIG_ENDIAN)
>> +#define __BYTE_ORDER __BIG_ENDIAN
>> +#endif
>> +#if defined(_LITTLE_ENDIAN)
>> +#define __BYTE_ORDER __LITTLE_ENDIAN
>> +#endif
>
> The existing support is only for platforms where all three macros
> (BYTE_ORDER, LITTLE_ENDIAN and BIG_ENDIAN) are defined, and the
> convention used on such platforms where BYTE_ORDER is set to either
> one of the *_ENDIAN macros to tell the code which byte order we
> have.  This mimics the convention where __BYTE_ORDER and other two
> macros are already defined with two leading underscores, and in such
> a case we do not have to do anything.  We make the final decision to
> use or bypass bswap64() in our ntohll() implementation based on the
> variables with double leading underscores.
>
> This patch seems to address two unrelated issues in that.
>
>  (1) The existing support does not help a platform where the
>      convention is to define either _BIG_ENDIAN (with one leading
>      underscore) or _LITTLE_ENDIAN and not both, which is Solaris
>      but there may be others.
>
>  (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already
>      defined on the platform.  Or these may not have been defined at
>      all.  You avoid unconditionally redefing these.
>
> I find the latter iffy.
>
> What is the reason for avoiding redefinition?  Is it because you
> know the original values they have are precious?  And if so in what
> way they are precious?  If the reason of avoiding redefinition is
> because you do not even know what their values are (so that you are
> trying to be safe by preserving), what other things can you say
> about their values you are preserving?
>
> Specifically, do you know that these two are defined differently, so
> that defining __BYTE_ORDER to one of them and comparing it to
> __BIG_ENDIAN is a good way to tell if the platform is big endian?
>
> I would understand it if (2) were "we undefine if these are defined
> and then define them as 4321 and 1234 respectively, in order to
> avoid a compiler warning against redefinition of a macro", but that
> is not what I am seeing, so I am not sure what you meant to achieve
> by that "if !defined()" constructs.
>
> Thanks.

> Just a thought.
>
> I am wondering if you may want to go the other way around.  That is,
> instead of using "we have byte-order, big and little and the way to
> determine endianness is to see byte-order matches which of the
> latter two", use "there may be either big or little but not both
> defined, and that is how you learn the byte-order".
>
> And make these two macros match what Solaris happens to use.
>
> I am not sure which variant I like better myself, though.
> ...

> 
> This patch seems to address two unrelated issues in that.
> 
>  (1) The existing support does not help a platform where the
>      convention is to define either _BIG_ENDIAN (with one leading
>      underscore) or _LITTLE_ENDIAN and not both, which is Solaris
>      but there may be others.
> 
>  (2) There may be __LITTLE_ENDIAN and __BIG_ENDIAN macros already
>      defined on the platform.  Or these may not have been defined at
>      all.  You avoid unconditionally redefing these.
> 
> I find the latter iffy.

>  compat/bswap.h | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/compat/bswap.h b/compat/bswap.h
> index 120c6c1..f08a9fe 100644
> --- a/compat/bswap.h
> +++ b/compat/bswap.h
> @@ -101,19 +101,34 @@ static inline uint64_t git_bswap64(uint64_t x)
>  #undef ntohll
>  #undef htonll
>  
> -#if !defined(__BYTE_ORDER)
> -# if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
> -#  define __BYTE_ORDER BYTE_ORDER
> -#  define __LITTLE_ENDIAN LITTLE_ENDIAN
> -#  define __BIG_ENDIAN BIG_ENDIAN
> +#if defined(BYTE_ORDER) && defined(LITTLE_ENDIAN) && defined(BIG_ENDIAN)
> +
> +# define GIT_BYTE_ORDER BYTE_ORDER
> +# define GIT_LITTLE_ENDIAN LITTLE_ENDIAN
> +# define GIT_BIG_ENDIAN BIG_ENDIAN
> +
> +#elif defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && defined(__BIG_ENDIAN)
> +
> +# define GIT_BYTE_ORDER __BYTE_ORDER
> +# define GIT_LITTLE_ENDIAN __LITTLE_ENDIAN
> +# define GIT_BIG_ENDIAN __BIG_ENDIAN
> +
> +#else
> +
> +# define GIT_BIG_ENDIAN 4321
> +# define GIT_LITTLE_ENDIAN 1234
> +
> +# if defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
> +#  define GIT_BYTE_ORDER GIT_BIG_ENDIAN
> +# elif defined(_BIG_ENDIAN) && !defined(_LITTLE_ENDIAN)
> +#  define GIT_BYTE_ORDER GIT_LITTLE_ENDIAN
> +# else
> +#  error "Cannot determine endianness"
>  # endif
> -#endif
>  
> -#if !defined(__BYTE_ORDER)
> -# error "Cannot determine endianness"
>  #endif
>  
> -#if __BYTE_ORDER == __BIG_ENDIAN
> +#if GIT_BYTE_ORDER == GIT_BIG_ENDIAN
>  # define ntohll(n) (n)
>  # define htonll(n) (n)
>  #else

> Charles Bailey <cbailey32@bloomberg.net> writes:
> 
> > ---
> 
> Please sign-off your patches ;-)

> Charles Bailey <cbailey32@bloomberg.net> writes:
>
>> I claim that any
>> platform which provides both but with differing senses is somewhat
>> broken so I cannot see the precedence mattering much.
>
> I agree with that, and that is the reason why we shouldn't change
> the order all of a sudden.  If it shouldn't matter, then there is
> only downside of a possiblity to break such an insane set-up that
> has been happily working by accident, without helping anybody if we
> change it, no?