Re: [PATCH] Ensure __BYTE_ORDER is always set

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 30, 2014, 22:02 UTC
Message-ID: <20140130220233.GH27577@google.com>
In-Reply-To: <1391111741-28994-1-git-send-email-brian@gernhardtsoftware.com>

Hi,

Brian Gernhardt wrote:

Show 10 quoted lines

> a201c20 (ewah: support platforms that require aligned reads) added a
> reliance on the existence of __BYTE_ORDER and __BIG_ENDIAN.  However,
> these macros are spelled without the leading __ on some platforms (OS
> X at least).  In this case, the endian-swapping code was added even
> when unnecessary, which caused assertion failures in
> t5310-pack-bitmaps.sh as the code that used the bitmap would read past
> the end.
>
> We already had code to handle this case in compat/bswap.h, but it was
> only used if we couldn't already find a reasonable version of bswap64.

Makes sense.  Sorry I missed this.

In an ideal world I would prefer to just rely on ntohll when it's decent (meaning that the '#if __BYTE_ORDER != __BIG_ENDIAN' block could be written as

	if (ntohll(1) != 1) {
		...
	}

or

	if (ntohll(1) == 1)
		; /* Big endian.  Nothing to do.
	else {
		...
	}

). But compat/bswap.h already relies on knowing the endianness at preprocessing time so that wouldn't buy anything.

Another "in an ideal world" option: make the loop unconditional after checking that optimizers on big-endian systems realize it's a noop. In any event, in the real world your patch looks like the right thing to do.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>