HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcppMinor

Merge sort in C++ with iterators

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withsortiteratorsmerge

Problem

I implemented merge sort with C++ and would like to get some feedback.

#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:

// 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.