Re: [PATCH 1/5] add SWAP macro
- From
Jeff King <peff@peff.net>
- Date
- Apr 24, 2017, 11:29 UTC
- Message-ID
- <20170424112928.rty5xejep4mnxph2@sigill.intra.peff.net>
- In-Reply-To
- <0bdb58a6-3a7f-2218-4b70-c591ae90e95e@web.de>
On Sat, Jan 28, 2017 at 10:38:21PM +0100, René Scharfe wrote:
Show 17 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)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/