-
Notifications
You must be signed in to change notification settings - Fork 2
Make dmsort work with non-default-constructible types (fixes #1) #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Nice find, and silly I'll repeat the benchmarks with this change later (in ~12h) (although it's far from the critical part of the algorithm, so it shouldn't matter anyway) and most likely merge it then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, the second overload (with-trivial-copies) may also need this, because a type may be trivially copyable and not be default constructible.
for (int i = 0; i < num_dropped_in_row; ++i) { | ||
--read; | ||
*read = std::move(*(dropped.end() - (i+1))); | ||
} | ||
dropped.resize(trunc_to_length); | ||
for (int i = 0; i < num_dropped_in_row; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can most likely be merged with the loop above:
for (int i = 0; i < num_dropped_in_row; ++i) {
--read;
*read = std::move(*(dropped.end() - 1));
dropped.pop_back();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we could try that too, that might be more sensible.
Eh, it's true that I didn't think of trivially copyable types that are not default-constructible, but I guess that such types can indeed exist. Good catch. |
By the way, it looks like the algorithm could easily be made to work with bidirectional iterators instead of random-access ones. The changes I see to make the magic happen:
Apparently, the algorithm almost never performs big jumps in the collection, so making it work with bidirectional iterators could be worth (unlike e.g. heapsort). You may want to consider it since there aren't many good sorting algorithms for bidirectional iterators :) My small tests on one million integer lists show that it's more than decent for bidirectional iterators too (ok, the more efficient would still probably be to move everything to a vector, sort it, and move everything back): |
Thank you. I'll take a look at bidirectional iterator changes too, thanks for that advice too! |
See issue #1 for the detail.