Re: [PATCH 1/5] add SWAP macro
- From
René Scharfe <l.s.r@web.de>
- Date
- Apr 28, 2017, 17:04 UTC
- Message-ID
- <070a4b85-98e0-12a4-6c9c-557f3dfa733c@web.de>
- In-Reply-To
- <20170424112928.rty5xejep4mnxph2@sigill.intra.peff.net>
Am 24.04.2017 um 13:29 schrieb Jeff King:
Show 33 quoted lines
> 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é