Re: [PATCH 1/5] add SWAP macro

From: Jeff King <peff@peff.net>
Date: Apr 28, 2017, 21:49 UTC
Message-ID: <20170428214934.tuqihgch6qeen3ni@sigill.intra.peff.net>
In-Reply-To: <070a4b85-98e0-12a4-6c9c-557f3dfa733c@web.de>

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

Show 16 quoted lines

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

Show 9 quoted lines

> 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