Re: [PATCH 1/5] add SWAP macro

[Jeff King <peff@peff.net>]

On Fri, Apr 28, 2017 at 07:04:51PM +0200, René Scharfe wrote:

> > 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.

No, I doubt that it's useful, and it's probably a sign of a bug
elsewhere. But it's likely a _harmless_ bug, so it may be irritating to
die when we hit it rather than continuing.

I dunno. I could go either way. Or we could leave it as-is, and let
valgrind find the problem. That has zero run-time cost, but of course
nobody bothers to run valgrind outside of the test suite, so the inputs
are not usually very exotic.

> 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?

Yes, the overlap case is probably an actual bug. Detecting it is a bit
harder, but definitely possible. I hate to pay the run-time cost for it,
but I wonder if a compiler could optimize it out.

> 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.

Yes, see my other reply. :)

-Peff