Re: [PATCH 1/5] add SWAP macro

[Jeff King <peff@peff.net>]

On Sat, Jan 28, 2017 at 10:38:21PM +0100, 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)

What should:

  SWAP(foo[i], foo[j]);

do when i == j? With this code, it ends up calling

  memcpy(&foo[i], &foo[j], ...);

which can cause valgrind to complain about overlapping memory. I suspect
in practice that noop copies are better off than partial overlaps, but I
think it does still violate the standard.

Is it worth comparing the pointers and bailing early?

A related question is whether the caller should ever be asking to swap
something with itself. This particular case[1] comes from
prio_queue_reverse(). I suspect its "<=" could become a "<", but I
haven't thought it through carefully.

-Peff

[1] http://public-inbox.org/git/CACsJy8AAtV5KJHBqWvnYb3Mw9CVzEdG3M-UJA+jd5MR5e-UMsA@mail.gmail.com/