Re: [PATCH v8 01/12] bswap: add 64 bit endianness helper get_be64

From: Jeff King <peff@peff.net>
Date: Sep 24, 2017, 03:51 UTC
Message-ID: <20170924035127.cwh54bjbbisl2dqf@sigill.intra.peff.net>
In-Reply-To: <MWHPR21MB047887A575D5DFFB213092A1F4640@MWHPR21MB0478.namprd21.prod.outlook.com>

On Sat, Sep 23, 2017 at 11:31:50PM +0000, Ben Peart wrote:

Show 15 quoted lines

> > diff --git a/compat/bswap.h b/compat/bswap.h index 6b22c4621..9dc79bdf5
> > 100644
> > --- a/compat/bswap.h
> > +++ b/compat/bswap.h
> > @@ -183,8 +183,8 @@ static inline uint32_t get_be32(const void *ptr)  static
> > inline uint64_t get_be64(const void *ptr)  {
> >  	const unsigned char *p = ptr;
> > -	return	(uint64_t)get_be32(p[0]) << 32 |
> > -		(uint64_t)get_be32(p[4]) <<  0;
> > +	return	(uint64_t)get_be32(p + 0) << 32 |
> > +		(uint64_t)get_be32(p + 4) <<  0;
> 
> This is surprising.  Every other function in the file uses the p[x] syntax.  Just for
> consistency, is there a way to stick to that syntax but still make it work correctly?
> Is there a typecast that can make it work?

The other ones are accessing the byte values directly, but since you are building on get_be32 here, you have to pass the pointer. So:

  return (uint64_t)get_be32(&p[0]) << 32 |
         (uint64_t)get_be32(&p[4]) <<  0;

would work.  Or of course you could just spell it out like the others:

  return (uint64_t)p[0] << 56 |
         (uint64_t)p[1] << 48 |
         (uint64_t)p[2] << 40 |
	 (uint64_t)p[3] << 32 |
         (uint64_t)p[4] << 24 |
         (uint64_t)p[5] << 16 |
         (uint64_t)p[6] <<  8 |
         (uint64_t)p[7] <<  0;

I suspect compilers would end up with the same output either way (on x86, "gcc -O2" actually turns the whole thing into a bswap instruction).

-Peff