snippetcppMinor
Merge sort in C++ with iterators
Viewed 0 times
withsortiteratorsmerge
Problem
I implemented merge sort with C++ and would like to get some feedback.
Usage:
Is there anything I should avoid? Any mistakes in the algorithm or implementation? It should work with other containers (e.g.
#include
#include
template
std::vector merge(const It begin, const It mid, const It end)
{
std::vector v;
It it_l{ begin }, it_r{ mid };
const It it_mid{ mid }, it_end{ end };
while (it_l != it_mid && it_r != it_end)
{
v.push_back((*it_l
void merge_sort(It begin, It end)
{
auto size = std::distance(begin, end);
if (size < 2)
return;
auto mid = std::next(begin, size / 2);
merge_sort(begin, mid);
merge_sort(mid, end);
auto &&v = merge(begin, mid, end);
std::move(v.cbegin(), v.cend(), begin);
}Usage:
std::vector v{ 8, 4, 1, 9, 16, 3 };
merge_sort(v.begin(), v.end());Is there anything I should avoid? Any mistakes in the algorithm or implementation? It should work with other containers (e.g.
std::list) as well.Solution
Very nice overall.
Don't see anything technically wrong.
I agree with @ratchet freak that your variable naming (and declaring multiple objects in one line) is a not great. I would prefer better names and one variable per line.
Things I would change for efficiency:
Also this is not what you want:
Otherwise you will screw up RVO done by the compiler and it will generate a copy rather than a move.
At the call site the value returned by a function is already an rvalue reference. So adding the move here does not change anything (it will still be moved).
But this is not going to move the values:
Here you are only getting lvalues passed to
Don't see anything technically wrong.
I agree with @ratchet freak that your variable naming (and declaring multiple objects in one line) is a not great. I would prefer better names and one variable per line.
Things I would change for efficiency:
// You know how big this vector is going to get.
std::vector v;
// So reserve the appropriate space:
v.reserve(std::distance(begin, end));Also this is not what you want:
return std::move(v);
// jsut return the object
return v;Otherwise you will screw up RVO done by the compiler and it will generate a copy rather than a move.
At the call site the value returned by a function is already an rvalue reference. So adding the move here does not change anything (it will still be moved).
But this is not going to move the values:
buffer.push_back((*left <= *right) ? *left++ : *right++);Here you are only getting lvalues passed to
push_back so you are copying the underlying elements. Why not try and get a move out of it?buffer.push_back(std::move((*left <= *right) ? *left++ : *right++));Code Snippets
// You know how big this vector is going to get.
std::vector<typename It::value_type> v;
// So reserve the appropriate space:
v.reserve(std::distance(begin, end));return std::move(v);
// jsut return the object
return v;buffer.push_back((*left <= *right) ? *left++ : *right++);buffer.push_back(std::move((*left <= *right) ? *left++ : *right++));Context
StackExchange Code Review Q#78129, answer score: 6
Revisions (0)
No revisions yet.