Re: pack bitmap woes on Windows

[Johannes Sixt <j.sixt@viscovery.net>]

Am 2/12/2014 17:48, schrieb Jeff King:
> On Wed, Feb 12, 2014 at 03:49:24PM +0100, Erik Faye-Lund wrote:
> 
>> It seems the author of a201c20b41a2f0725977bcb89a2a66135d776ba2
>> assumes __BYTE_ORDER was guaranteed to always be defined, probably
>> fooled by the following check:
>>
>> #if !defined(__BYTE_ORDER)
>> # error "Cannot determine endianness"
>> #endif
>>
>> However, that check is only performed if we're NOT building with GCC
>> on x86/x64 nor MSVC (which I don't think defined __BYTE_ORDER either)
>>
>> The following would make __BYTE_ORDER a guarantee, and show that MinGW
>> does not define it:
> 
> Yes, that is the problem. Sorry, this got brought up earlier[1], but the
> discussion went on and I did not notice that Brian's patch did not get
> applied.
> 
> After re-reading the discussion, I think the patch below is the best
> solution. Can you confirm that it fixes the problem for you?

Yes, it fixes the problem! t5310-pack-bitmaps passes all tests with the patch.

Thanks,
-- Hannes

> 
> -Peff
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/241278
> 
> -- >8 --
> Subject: ewah: unconditionally ntohll ewah data
> 
> Commit a201c20 tried to optimize out a loop like:
> 
>   for (i = 0; i < len; i++)
> 	  data[i] = ntohll(data[i]);
> 
> in the big-endian case, because we know that ntohll is a
> noop, and we do not need to pay the cost of the loop at all.
> However, it mistakenly assumed that __BYTE_ORDER was always
> defined, whereas it may not be on systems which do not
> define it by default, and where we did not need to define it
> to set up the ntohll macro. This includes OS X and Windows.
> 
> We could muck with the ordering in compat/bswap.h to make
> sure it is defined unconditionally, but it is simpler to
> still to just execute the loop unconditionally. That avoids
> the application code knowing anything about these magic
> macros, and lets it depend only on having ntohll defined.
> 
> And since the resulting loop looks like (on a big-endian
> system):
> 
>   for (i = 0; i < len; i++)
> 	  data[i] = data[i];
> 
> any decent compiler can probably optimize it out.
> 
> Original report and analysis by Brian Gernhardt.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  ewah/ewah_io.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
> index 4a7fae6..f7f700e 100644
> --- a/ewah/ewah_io.c
> +++ b/ewah/ewah_io.c
> @@ -113,6 +113,7 @@ int ewah_serialize(struct ewah_bitmap *self, int fd)
>  int ewah_read_mmap(struct ewah_bitmap *self, void *map, size_t len)
>  {
>  	uint8_t *ptr = map;
> +	size_t i;
>  
>  	self->bit_size = get_be32(ptr);
>  	ptr += sizeof(uint32_t);
> @@ -135,13 +136,8 @@ int ewah_read_mmap(struct ewah_bitmap *self, void *map, size_t len)
>  	memcpy(self->buffer, ptr, self->buffer_size * sizeof(uint64_t));
>  	ptr += self->buffer_size * sizeof(uint64_t);
>  
> -#if __BYTE_ORDER != __BIG_ENDIAN
> -	{
> -		size_t i;
> -		for (i = 0; i < self->buffer_size; ++i)
> -			self->buffer[i] = ntohll(self->buffer[i]);
> -	}
> -#endif
> +	for (i = 0; i < self->buffer_size; ++i)
> +		self->buffer[i] = ntohll(self->buffer[i]);
>  
>  	self->rlw = self->buffer + get_be32(ptr);
>  
>