Web lists-archives.com

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




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

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