Am 24.04.2017 um 13:29 schrieb Jeff King:
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?
Hmm, so swapping a value with itself can be a useful thing to do?
Otherwise an assert would be more appropriate.
Swapping with *partial* overlap sounds tricky, or even evil. If
we want to support that for some reason we'd have to use memmove
in the middle. But that would still corrupt at least one of the
objects, wouldn't it?
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.
The line in question is this one:
for (i = 0; i <= (j = (queue->nr - 1) - i); i++)
Assignment in the middle? Hmm. Why not do it like this?
for (i = 0, j = queue->nr - 1; i < j; i++, j--)
Looks less complicated to me.
René
