Re: [PATCH] Ensure __BYTE_ORDER is always set

From: Jeff King <peff@peff.net>
Date: Jan 30, 2014, 22:46 UTC
Message-ID: <20140130224657.GA30478@sigill.intra.peff.net>
In-Reply-To: <20140130220233.GH27577@google.com>

On Thu, Jan 30, 2014 at 02:02:33PM -0800, Jonathan Nieder wrote:

Show 18 quoted lines

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

Yes, though it would simplify things because we are depending on ntohll being defined, rather than some obscure macros.

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

I had the same thought when reading the original patch. The loop after pre-processing on a big-endian system should look like:

  {
          size_t i;
          for (i = 0; i < self->buffer_size; ++i)
              self->buffer[i] = self->buffer[i];
  }

It really seems like the sort of thing that any halfway decent compiler should be able to turn into a noop. I'm OK to go that route, and if you don't have a halfway decent compiler, tough cookies; git will waste your precious nanoseconds doing a relatively small loop. If this loop actually mattered, we would probably do better still to leave it in disk order, and fix it up as-needed only when we look at a particular bitmap (we do not typically need to look at all of them on disk).

-Peff