From: René Scharfe <l.s.r@web.de>
Date: Mon, 30 Jan 2017 16:59:37 GMT
Subject: Re: [PATCH 1/5] add SWAP macro
Message-ID: <11e27272-c1cd-a127-eb44-385d827af63d@web.de>
In-Reply-To: <alpine.DEB.2.20.1701301643260.3469@virtualbox>

Am 30.01.2017 um 17:01 schrieb Johannes Schindelin:
> Hi René,
>
> On Sat, 28 Jan 2017, René Scharfe wrote:
>
>> 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)

We can move the underscore to the end, but using a and b directly will 
give surprising results if the parameters have side effects.  E.g. if 
you want to swap the first two elements of two arrays you might want to 
do this:

	SWAP(*x++, *y++);
	SWAP(*x++, *y++);

And that would increment twice as much as one would guess and access 
unexpected elements.

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

I wouldn't worry too much about such a solution before seeing that SWAP 
(even with memcpy(3) -- this function is probably optimized quite 
heavily on most platforms) causes an actual performance problem.

René