std.algorithm: Remove the hasAssignableElements restriction from HeapSortImpl #1858
Conversation
|
While trying to write a test for this, I discovered that the |
|
a) I think it is an "unforeseen consequence". I wouldn't lose any sleep over this one.
Hum... you are right, I didn't see any direct calls to |
This restriction seems unnecessary (as HeapSortImpl uses swap/swapAt, which does not require assignability), and was getting in the way of e.g. sorting a range of Refs. The range can satisfy either hasAssignableElements, or hasSwappableElements
Ah yes, good catch. Updated with |
| @@ -2039,7 +2039,7 @@ if (allMutableFields!T && !is(typeof(T.init.proxySwap(T.init)))) | |||
| } | |||
|
|
|||
| // Not yet documented | |||
| void swap(T)(T lhs, T rhs) if (is(typeof(T.init.proxySwap(T.init)))) | |||
| void swap(T)(auto ref T lhs, auto ref T rhs) if (is(typeof(lhs.proxySwap(rhs)))) | |||
There was a problem hiding this comment.
Absolutely not. Swap takes by reference, always. You can't swap args that are passed by value.
There was a problem hiding this comment.
You can if the type being swapped only contains a pointer towards the real data. This is the case in what I wanted to accomplish when I wrote this pull: http://stackoverflow.com/a/21104053/21501
There was a problem hiding this comment.
The differentiator is whether proxySwap takes rhs by value or reference. If it takes it by reference, this instantiation will fail if auto ref resolves to non-ref. If it takes it by value, then it doesn't need to mutate the other value, possibly only its indirections.
There was a problem hiding this comment.
If it takes it by reference, this instantiation will fail if auto ref resolves to non-ref.
Oh, gah, not it won't! It'll get a copy. I see what you mean now. swap here needs to accept either ref, or a qualifier that is not passable to a ref function. Would ref and in overloads work?
There was a problem hiding this comment.
Would
refandinoverloads work?
in means const, so that wouldn't work (mutation of const). "ref scope" could work, but (AFAIK) that doesn't work for some reason. As for "scope", are the references ever escaped? They are swapped, that's for sure...
In nay case (AFAIK), scope don't work right now, so it might be best to simply leave it as ref.
|
LGTM, though I still have dounts about how "official" |
…nable std.algorithm: Remove the hasAssignableElements restriction from HeapSortImpl
This restriction seems unnecessary (as
HeapSortImplusesswap/swapAt, which does not require assignability), and was getting in the way of e.g. sorting a range ofRefs.This was the only thing preventing the sorting of a range of structs with disabled
opAssign, but implementingproxySwap.