Re: [PATCH 1/5] add SWAP macro

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 30, 2017, 16:01 UTC
Message-ID: <alpine.DEB.2.20.1701301643260.3469@virtualbox>
In-Reply-To: <0bdb58a6-3a7f-2218-4b70-c591ae90e95e@web.de>

Hi René,

On Sat, 28 Jan 2017, René Scharfe wrote:

Show 19 quoted lines

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 87237b092b..66cd466eea 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char *suffix)
>  	return strip_suffix(str, suffix, &len);
>  }
>  
> +#define SWAP(a, b) do {						\
> +	void *_swap_a_ptr = &(a);				\
> +	void *_swap_b_ptr = &(b);				\
> +	unsigned char _swap_buffer[sizeof(a)];			\
> +	memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));		\
> +	memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +		\
> +	       BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));	\
> +	memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));		\
> +} while (0)
> +
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)

It may seem as a matter of taste, or maybe not: I prefer this without the _swap_a_ptr (and I would also prefer not to use identifiers starting with an underscore, as section 7.1.3 Reserved Identifiers of the C99 standard says they are reserved):

+#define SWAP(a, b) do { \ + unsigned char swap_buffer_[sizeof(a)]; \ + memcpy(swap_buffer_, &(a), sizeof(a)); \ + memcpy(&(a), &(b), sizeof(a) + \ + BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b))); \ + memcpy(&(b), swap_buffer_, sizeof(a)); \ +} while (0)

One idea to address the concern that not all C compilers people use to build Git may optimize away those memcpy()s: we could also introduce a SWAP_PRIMITIVE_TYPE (or SWAP2 or SIMPLE_SWAP or whatever) that accepts only primitive types. But since __typeof__() is not portable...

Ciao, Dscho