Skip to main content
Fix grammar/formating and remove rant (I liked it but edited the question to remove line numbers, so...).
Source Link
Morwenn
  • 20k
  • 3
  • 67
  • 127

It would have been better if you had used iterators to implement the sort. ItsIt's a lot more versatile than using a specific container.

38 int_v::iterator m = v.begin() + v.size()/2; 39 int_v l(v.begin(), m); 40 int_v r(m, v.end());

int_v::iterator m = v.begin() + v.size()/2;
int_v l(v.begin(), m);
int_v r(m, v.end());

Each recursive call makemakes a copy of the data to be merged. So we get this progression.:

So you are using 2n\$2n\$ size of data just through calling MergeSortMergeSort and it is being allocated and deallocated as the std::vectorstd::vector is being used then destroyed on return.

In Addition you are doing another memory allocation in Merge():

Note:

By using iterators rather than containers. It, it becomes easy to avoid some of this copying. You just pass the ranges of the containers and do it in-place.

Prefer to move rather than to copy.

30   vec = res;

Here you are copying the contentcontents of the array. Arrays can be moved (this means it swaps a couple of pointers rather than copying all the data). Since you are not using res after this point you should move it.

30   vec = std::move(res);

User defined types usually start with a capitolcapital

To distinguish user defined types and objects. Proceed types with an initial capitolcapital letter.

Personal

I hated that you posted line numbers into the question.
That kind of things makes it hard to cut and paste your code into our editors for review.

Also if SE sites decide to add line numbers to their code your extra line numbers will become an eye sore.

It would have been better if you had used iterators to implement the sort. Its a lot more versatile than using a specific container.

38 int_v::iterator m = v.begin() + v.size()/2; 39 int_v l(v.begin(), m); 40 int_v r(m, v.end());

Each recursive call make a copy of the data to be merged. So we get this progression.

So you are using 2n size of data just through calling MergeSort and it is being allocated and deallocated as the std::vector is being used then destroyed on return.

In Addition you are doing another memory allocation in Merge()

Note:

By using iterators rather than containers. It becomes easy to avoid some of this copying. You just pass the ranges of the containers and do it in-place.

Prefer to move rather than copy.

30   vec = res;

Here you are copying the content of the array. Arrays can be moved (this means it swaps a couple of pointers rather than copying all the data). Since you are not using res after this point you should move it.

30   vec = std::move(res);

User defined types usually start with a capitol

To distinguish user defined types and objects. Proceed types with an initial capitol letter.

Personal

I hated that you posted line numbers into the question.
That kind of things makes it hard to cut and paste your code into our editors for review.

Also if SE sites decide to add line numbers to their code your extra line numbers will become an eye sore.

It would have been better if you had used iterators to implement the sort. It's a lot more versatile than using a specific container.

int_v::iterator m = v.begin() + v.size()/2;
int_v l(v.begin(), m);
int_v r(m, v.end());

Each recursive call makes a copy of the data to be merged. So we get this progression:

So you are using \$2n\$ size of data just through calling MergeSort and it is being allocated and deallocated as the std::vector is being used then destroyed on return.

In Addition you are doing another memory allocation in Merge():

Note

By using iterators rather than containers, it becomes easy to avoid some of this copying. You just pass the ranges of the containers and do it in-place.

Prefer to move rather than to copy

vec = res;

Here you are copying the contents of the array. Arrays can be moved (this means it swaps a couple of pointers rather than copying all the data). Since you are not using res after this point you should move it.

vec = std::move(res);

User defined types usually start with a capital

To distinguish user defined types and objects. Proceed types with an initial capital letter.

added 449 characters in body
Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338

Overall comment.

It would have been better if you had used iterators to implement the sort. Its a lot more versatile than using a specific container.

Memory Allocation

Your implementation requires a lot of dynamic memory allocation.

38 int_v::iterator m = v.begin() + v.size()/2; 39 int_v l(v.begin(), m); 40 int_v r(m, v.end());

Each recursive call make a copy of the data to be merged. So we get this progression.

    n + n/2 + n/4 + n/8 + n/16 ....
    =>  n(1 + 1/2 + 1/4 + 1/8 + 1/16.....)
    =>  2n

So you are using 2n size of data just through calling MergeSort and it is being allocated and deallocated as the std::vector is being used then destroyed on return.

In Addition you are doing another memory allocation in Merge()

int_v res;
res.reserve(l.size() + r.size());

Note:

By using iterators rather than containers. It becomes easy to avoid some of this copying. You just pass the ranges of the containers and do it in-place.

Prefer to move rather than copy.

30   vec = res;

Here you are copying the content of the array. Arrays can be moved (this means it swaps a couple of pointers rather than copying all the data). Since you are not using res after this point you should move it.

30   vec = std::move(res);

User defined types usually start with a capitol

typedef std::vector<int> int_v;

To distinguish user defined types and objects. Proceed types with an initial capitol letter.

Personal

I hated that you posted line numbers into the question.
That kind of things makes it hard to cut and paste your code into our editors for review.

Also if SE sites decide to add line numbers to their code your extra line numbers will become an eye sore.

My Interface would have been:

template<typename I>
void mergeSort(I begin, I end)
{
    auto size = std::distance(begin, end);
    auto mid  = begin;
    std::advance(mid, size/2);

    mergeSort(begin, mid);
    mergeSort(mid, end);
    merge(begin, mid, end);
}
template<typename C>
void mergeSort(C& cont)
{
    mergeSort(std::begin(cont), std::end(cont));
}

Overall comment.

It would have been better if you had used iterators to implement the sort. Its a lot more versatile than using a specific container.

Memory Allocation

Your implementation requires a lot of dynamic memory allocation.

38 int_v::iterator m = v.begin() + v.size()/2; 39 int_v l(v.begin(), m); 40 int_v r(m, v.end());

Each recursive call make a copy of the data to be merged. So we get this progression.

    n + n/2 + n/4 + n/8 + n/16 ....
    =>  n(1 + 1/2 + 1/4 + 1/8 + 1/16.....)
    =>  2n

So you are using 2n size of data just through calling MergeSort and it is being allocated and deallocated as the std::vector is being used then destroyed on return.

In Addition you are doing another memory allocation in Merge()

int_v res;
res.reserve(l.size() + r.size());

Note:

By using iterators rather than containers. It becomes easy to avoid some of this copying. You just pass the ranges of the containers and do it in-place.

Prefer to move rather than copy.

30   vec = res;

Here you are copying the content of the array. Arrays can be moved (this means it swaps a couple of pointers rather than copying all the data). Since you are not using res after this point you should move it.

30   vec = std::move(res);

User defined types usually start with a capitol

typedef std::vector<int> int_v;

To distinguish user defined types and objects. Proceed types with an initial capitol letter.

Personal

I hated that you posted line numbers into the question.
That kind of things makes it hard to cut and paste your code into our editors for review.

Also if SE sites decide to add line numbers to their code your extra line numbers will become an eye sore.

Overall comment.

It would have been better if you had used iterators to implement the sort. Its a lot more versatile than using a specific container.

Memory Allocation

Your implementation requires a lot of dynamic memory allocation.

38 int_v::iterator m = v.begin() + v.size()/2; 39 int_v l(v.begin(), m); 40 int_v r(m, v.end());

Each recursive call make a copy of the data to be merged. So we get this progression.

    n + n/2 + n/4 + n/8 + n/16 ....
    =>  n(1 + 1/2 + 1/4 + 1/8 + 1/16.....)
    =>  2n

So you are using 2n size of data just through calling MergeSort and it is being allocated and deallocated as the std::vector is being used then destroyed on return.

In Addition you are doing another memory allocation in Merge()

int_v res;
res.reserve(l.size() + r.size());

Note:

By using iterators rather than containers. It becomes easy to avoid some of this copying. You just pass the ranges of the containers and do it in-place.

Prefer to move rather than copy.

30   vec = res;

Here you are copying the content of the array. Arrays can be moved (this means it swaps a couple of pointers rather than copying all the data). Since you are not using res after this point you should move it.

30   vec = std::move(res);

User defined types usually start with a capitol

typedef std::vector<int> int_v;

To distinguish user defined types and objects. Proceed types with an initial capitol letter.

Personal

I hated that you posted line numbers into the question.
That kind of things makes it hard to cut and paste your code into our editors for review.

Also if SE sites decide to add line numbers to their code your extra line numbers will become an eye sore.

My Interface would have been:

template<typename I>
void mergeSort(I begin, I end)
{
    auto size = std::distance(begin, end);
    auto mid  = begin;
    std::advance(mid, size/2);

    mergeSort(begin, mid);
    mergeSort(mid, end);
    merge(begin, mid, end);
}
template<typename C>
void mergeSort(C& cont)
{
    mergeSort(std::begin(cont), std::end(cont));
}
Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338

Overall comment.

It would have been better if you had used iterators to implement the sort. Its a lot more versatile than using a specific container.

Memory Allocation

Your implementation requires a lot of dynamic memory allocation.

38 int_v::iterator m = v.begin() + v.size()/2; 39 int_v l(v.begin(), m); 40 int_v r(m, v.end());

Each recursive call make a copy of the data to be merged. So we get this progression.

    n + n/2 + n/4 + n/8 + n/16 ....
    =>  n(1 + 1/2 + 1/4 + 1/8 + 1/16.....)
    =>  2n

So you are using 2n size of data just through calling MergeSort and it is being allocated and deallocated as the std::vector is being used then destroyed on return.

In Addition you are doing another memory allocation in Merge()

int_v res;
res.reserve(l.size() + r.size());

Note:

By using iterators rather than containers. It becomes easy to avoid some of this copying. You just pass the ranges of the containers and do it in-place.

Prefer to move rather than copy.

30   vec = res;

Here you are copying the content of the array. Arrays can be moved (this means it swaps a couple of pointers rather than copying all the data). Since you are not using res after this point you should move it.

30   vec = std::move(res);

User defined types usually start with a capitol

typedef std::vector<int> int_v;

To distinguish user defined types and objects. Proceed types with an initial capitol letter.

Personal

I hated that you posted line numbers into the question.
That kind of things makes it hard to cut and paste your code into our editors for review.

Also if SE sites decide to add line numbers to their code your extra line numbers will become an eye sore.