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@xxxxxxxxxxxxxx/