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
