CODEWITHSUNDEEP
c++stlstd
Nikita Albekov
Nikita Albekov

Reputation: 85

How to use std::make_move_iterator in std::copy algorithm?

I have an issue:

When I am trying to move data from (vector with unique_ptr) to (list with unique_ptr) by std::copy algorithm is sucesfully compliles but then crushes in running! Here is a code:

std::vector<std::unique_ptr<std::string>> vecUPTRstring;
std::unique_ptr<std::string> ptrStringArr1(new std::string("qwe"));
std::unique_ptr<std::string> ptrStringArr2(new std::string("asd"));
vecUPTRstring.push_back(std::move(ptrStringArr1));
vecUPTRstring.push_back(std::move(ptrStringArr2));

std::list<std::unique_ptr<std::string>> listUPTRstring;

std::copy(std::make_move_iterator(vecUPTRstring.begin()),
                    std::make_move_iterator(vecUPTRstring.end()),
                    std::make_move_iterator(listUPTRstring.begin())
                );
for (auto& var : listUPTRstring)
{
    cout << "list value = " << var << endl;
}

The crash error is:

Expression: cannot dereference end list iterator

I have checked different ways to fix it and read some articles including this one, but I still need help with solving this problem.

Upvotes: 3

Views: 1321

Answers (2)

Kuba hasn&#39;t forgotten Monica
Kuba hasn&#39;t forgotten Monica

Reputation: 98425

The list holds values using a pointer-like indirection, and storing small elements like pointers in std::list is pretty much never faster than using a vector, with very few exceptions. So, to begin, you should check if using std::list actually saves you any time. You should also check if using std::unique_ptr is saving you any time.

Introduce indirection only when benchmarks prove improved performance when using such indirection. By default, keep things in vectors by value. Use other containers and layers of memory management only when measurements show benefit. And I do mean measurements. Modern CPU's performance is usually bound by memory access patterns, and cache misses can easily swamp any perceived "overhead" of "wastefully" copying contiguous cache lines. To a first approximation, sequential memory access is free.

When it comes to strings specifically, the following types perform from best to worst, with the last one being quite abysmal, especially when the strings are small:

  • std::vector<std::string>
  • std::vector<std::unique_ptr<std::string>>
  • std::list<std::string>
  • std::list<std::unique_ptr<std::string>>

But if you really wanted to use a list, and do lots of inserts/removes later in the list's life (the only reason ever to use that data type anyway), it may well turn out to be cheaper to pay the cost to move/copy the values into the list once, and then enjoy the benefits of lower indirection by using values, not explicit pointers.

In any case, you should definitely benchmark all of this, because in most cases std::list is something that makes sense on microcontrollers with no cache, and as soon as you run in the context of a large application and an active heap on a PC-class CPU made sometime in the last two decades, it turns into a performance disaster.

template <typename T>
std::list<T> ptrVectorToList(std::vector<std::unique_ptr<T>> && src)
{
  std::list<T> list;
  for (auto &ptr : src) {
    list.emplace_back(std::move(*ptr));
    ptr.reset();
  }
  src.clear();
  return list;
}

Upvotes: 1

orlp
orlp

Reputation: 117681

Only make the source iterators move iterator, and the destination iterator should be an inserter instead, since your list is empty. You would only pass dest.begin() as your destination if you already had n elements in your destination container and wish to overwrite them with the source elements.

std::copy(std::make_move_iterator(vecUPTRstring.begin()),
          std::make_move_iterator(vecUPTRstring.end()),
          std::inserter(listUPTRstring, listUPTRstring.end()));

However, I must also suggest std::move instead of std::copy with move iterators as it's semantically equal and simpler/clearer:

std::move(vecUPTRstring.begin(),
          vecUPTRstring.end(),
          std::inserter(listUPTRstring, listUPTRstring.end()));

Upvotes: 4

Related Questions