Re: [PATCH v1 2/5] Teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

From: Jeff King <peff@peff.net>
Date: May 16, 2017, 17:49 UTC
Message-ID: <20170516174948.arqbk533pihm6x46@sigill.intra.peff.net>
In-Reply-To: <29122818-71fb-5af9-59b1-03387f014151@gmail.com>

On Tue, May 16, 2017 at 01:17:56PM -0400, Ben Peart wrote:

Show 25 quoted lines

> > Thanks for the pointers.  I'll update this to use the existing get_be32
> > and have created a get_be64 and will use that for the last_update.
> 
> OK, now I'm confused as to the best path for adding a get_be64.  This one is
> trivial:
> 
> #define get_be64(p)	ntohll(*(uint64_t *)(p))
> 
> but should the unaligned version be:
> 
> #define get_be64(p)	( \
> 	(*((unsigned char *)(p) + 0) << 56) | \
> 	(*((unsigned char *)(p) + 1) << 48) | \
> 	(*((unsigned char *)(p) + 2) << 40) | \
> 	(*((unsigned char *)(p) + 3) << 32) | \
> 	(*((unsigned char *)(p) + 4) << 24) | \
> 	(*((unsigned char *)(p) + 5) << 16) | \
> 	(*((unsigned char *)(p) + 6) <<  8) | \
> 	(*((unsigned char *)(p) + 7) <<  0) )
> 
> or would it be better to do it like this:
> 
> #define get_be64(p)	( \
> 	((uint64_t)get_be32((unsigned char *)(p) + 0) << 32) | \
> 	((uint64_t)get_be32((unsigned char *)(p) + 4) <<  0)

I'd imagine the compiler would generate quite similar code between the two, and the second is much shorter and easier to read, so I'd probably prefer it.

> or with a static inline function like git_bswap64:

Try "git log -Sinline compat/bswap.h", which turns up the history of why it went from a macro to an inline function.

The get_be macros are simple enough that they can remain as macros, though I'd have no objection personally to them being inline functions. I'd expect modern compilers to be able to optimize similarly, and it removes the restriction that you can't call the macro with an argument that has side effects.

-Peff