Re: [PATCH] Add extra logic required to detect endianness on Solaris
On Thu, May 01, 2014 at 11:58:26AM -0700, Junio C Hamano wrote: 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. Yes, you are right. I think I was uncomfortable defining macros with names reserved for the implementation even if the implementation didn't seem to be using them. I think I made my patch less correct as a result. Looking at the rest of the git source code we don't seem to use any of these macros anywhere else so perhaps we could use macros specific to GIT? Let me follow up with an alternative patch. Charles. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add extra logic required to detect endianness on Solaris
Signed-off-by: Charles Bailey cbaile...@bloomberg.net --- The endian detection added in 7e3dae494 isn't sufficient for the Solaris Studio compilers. This adds some fallback logic which works for Solaris but would also work for AIX and Linux if it were needed. compat/bswap.h | 21 + 1 file changed, 21 insertions(+) diff --git a/compat/bswap.h b/compat/bswap.h index 120c6c1..5a41311 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -110,6 +110,27 @@ static inline uint64_t git_bswap64(uint64_t x) #endif #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 + +#endif /* !defined(__BYTE_ORDER) */ + +#if !defined(__BYTE_ORDER) # error Cannot determine endianness #endif -- 1.9.0 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add extra logic required to detect endianness on Solaris
Charles Bailey cbaile...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add extra logic required to detect endianness on Solaris
Junio C Hamano gits...@pobox.com writes: Charles Bailey cbaile...@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. compat/bswap.h | 21 + 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/compat/bswap.h b/compat/bswap.h index 120c6c1..e87998e 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -101,19 +101,24 @@ 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 -# endif +#if !defined(_BIG_ENDIAN) !defined(_LITTLE_ENDIAN) + +#if defined(BYTE_ORDER) defined(LITTLE_ENDIAN) defined(BIG_ENDIAN) +# if BYTE_ORDER == BIG_ENDIAN +# define _BIG_ENDIAN +# else +# define _LITTLE_ENDIAN +#endif + #endif -#if !defined(__BYTE_ORDER) +#if !defined(_BIG_ENDIAN) !defined(_LITTLE_ENDIAN) # error Cannot determine endianness +#elif defined(_BIG_ENDIAN) defined(_LITTLE_ENDIAN) +# error Your endianness is screwed up #endif -#if __BYTE_ORDER == __BIG_ENDIAN +#if defined (_BIG_ENDIAN) # define ntohll(n) (n) # define htonll(n) (n) #else -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add extra logic required to detect endianness on Solaris
Junio C Hamano gits...@pobox.com writes: 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. ... The how-about-this patch in the pregvious message forgets the default case where byte-order, little and big are defined with leading double underscore, which must also be handled, if we want to take this aproach. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
