Fix issue - sort should move elements instead of copy... #7524
Conversation
MoonlightSentinel
changed the title
MoonlightSentinel
force-pushed
the
move-sort
branch
from
15218d6 to
07908c4
Compare
|
Is this going to fix the |
|
Yes, because it won't call |
MoonlightSentinel
force-pushed
the
move-sort
branch
from
07908c4 to
5ff7bd5
Compare
Do a manual `schwartzSort` without using `Tuple` until all host compilers have the fix implemented in dlang/phobos#7524.
MoonlightSentinel
force-pushed
the
move-sort
branch
2 times, most recently
from
68b2c72 to
3d83b0d
Compare
Do a manual `schwartzSort` without using `Tuple` until all host compilers have the fix implemented in dlang/phobos#7524.
MoonlightSentinel
force-pushed
the
move-sort
branch
3 times, most recently
from
ea84c8c to
9f70bd9
Compare
|
It's all green. |
MoonlightSentinel
requested review from
andralex,
JackStouffer and
wilzbach
as code owners
std/algorithm/sorting.d
Outdated
| /// Helper method that moves from[fIdx] into to[tIdx] if both are lvalues and | ||
| /// uses a plain assignment if not (necessary for backwards compatibility) | ||
| void moveEntry(X, Y)(ref X from, const size_t fIdx, ref Y to, const size_t tIdx) | ||
| do |
There was a problem hiding this comment.
Forgot to remove do as well. Slices and decent range implementations will check the index anyway.
MoonlightSentinel
force-pushed
the
move-sort
branch
from
9f70bd9 to
d82eaba
Compare
|
Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#7524" |
... because it's reordering elements instead. Calling `opAssign` on unitialized members might also cause errors if the implementation requires a valid instance (see `TimSortImpl`)
MoonlightSentinel
force-pushed
the
move-sort
branch
from
d82eaba to
574f1f5
Compare
| alias lessEqual = (a, b) => !less(b, a); | ||
| bool greater()(auto ref T a, auto ref T b) { return less(b, a); } | ||
| bool greaterEqual()(auto ref T a, auto ref T b) { return !less(a, b); } | ||
| bool lessEqual()(auto ref T a, auto ref T b) { return !less(b, a); } |
There was a problem hiding this comment.
Not introduced here, but it seems these are superfluous (everything can be elegantly expressed with less). greater is not even used.
There was a problem hiding this comment.
I would agree for greaterEqual(a, b) vs !less(a,b). However, not for lessEqual(a, b) vs !less(b, a). More state for brain to track.
Do a manual `schwartzSort` without using `Tuple` until all host compilers have the fix implemented in dlang/phobos#7524.
The schwartzSort() issue has been fixed in Phobos master (dlang/phobos#7524, will land in 2.094) and worked around in the 2.093 run.d testrunner. The tests all pass, so make it a requirement.
... because it's reordering elements. Calling
opAssignon unitialized members might also cause errors if the implementation requires a valid instance (seeTimSortImpl)