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é

Reply via email to