0

Hello I am trying to understand how optimize moving elements from a vector to another checking if that is calling copy or move constructor on the way.

However when I try to erase the elements from a std::vector v1 that I moved out onto another vector v2, the compilers complains with the following message:

vector-move-elements.cpp:19:5: note: copy assignment operator is implicitly deleted because 'Foo' has a
      user-declared move constructor
    Foo(Foo&& other) {
    ^

Here is the code

#include <vector>
#include <string>
#include <iostream>
// #include <algorithm> std::copy_if

// inspired from https://stackoverflow.com/questions/15004517/moving-elements-from-stdvector-to-another-one
struct Foo
{
    Foo(std::string&& s, uint32_t idx) {
        this->name = std::move(s);
        this->index = std::move(idx);
        std::cout << "Calling default constructor " << name << idx << std::endl;
    }
    Foo(const Foo& other) {
        this->name = other.name;
        this->index = other.index;
        std::cout << "Calling copy constructor " << other.name << other.index << std::endl;
    }
    Foo(Foo&& other) {
        this->name = std::move(other.name);
        this->index = std::move(other.index);
        std::cout << "Calling move constructor " << other.name << other.index << std::endl;
    }
    std::string name;
    uint32_t index;
};

int main() {
    std::vector<Foo> v1, v2;
    v1.reserve(25); // https://stackoverflow.com/questions/52109542/push-back-to-stdvector-the-copy-constructor-is-repeatedly-called
    v2.reserve(25); // without this the push_back instruction causes a call to copy constructor

    // fill v1 with dummy elements
    for (uint32_t i = 0; i < 25; ++i ) {
        std::string s = std::string("name_") + std::to_string(i);
        std::cout << s << std::endl;
        v1.emplace_back(Foo(std::move(s), i));
    }

    std::cout << "End of construction" << std::endl;

    // populate v1 with at least 17 elements...
    const auto it = std::next(v1.begin(), 17); // same as v1.begin()+17;
    std::move(v1.begin(), it, std::back_inserter(v2));
    // std::copy_if(std::make_move_iterator(begin(v1)),
    //          std::make_move_iterator(end(v1)),
    //          std::back_inserter(v2),
    //          [](const Foo& v){ return v.index <= 17; });

    // this line DOESN'T COMPILE 
    v1.erase(v1.begin(), it); // copy assignment operator is implicitly deleted because 'Foo' has a
      user-declared move constructor

    // if I don't loop on auto& the for loop will call copy constructor
    for (auto const& e : v1) {
        std::cout << "v1: " << e.name << " " << e.index << std::endl;
    }
    for (auto const& e : v2) {
        std::cout << "v2: " << e.name << " " << e.index << std::endl;
    }
}
Oblivion
  • 7,176
  • 2
  • 14
  • 33
mpsido
  • 167
  • 4
  • The error message is pretty clear. You need to declare your own [copy assignment operator](https://en.cppreference.com/w/cpp/language/copy_assignment). – Max Vollmer Sep 22 '19 at 11:38
  • `this->index = std::move(other.index);` the `move` serves no purpose. And why isn't the code using the constructor's member initializer lists? – Eljay Sep 22 '19 at 11:56

1 Answers1

0

What you need to check is the rule of three/five/zero.

You need a copy assignment operator to get your code compiled:

Foo& operator=(const Foo& other) {
    this->name = other.name;
    this->index = other.index;
    std::cout << "Calling copy assignment " << other.name << other.index << std::endl;
    return *this;
}

and a move assignment operator to adhere to the rule of 5.

Live on godbolt

Oblivion
  • 7,176
  • 2
  • 14
  • 33
  • 1
    And a move assignment operator to adhere to the [rule of 5](https://en.cppreference.com/w/cpp/language/rule_of_three), – Paul Sanders Sep 22 '19 at 11:42
  • Good news is : IT WORKED ! But I don't understand why: I am erasing elements why is it requiring a `operator=`. – mpsido Sep 22 '19 at 11:42
  • I got it: in the std::vector::erase implementation the elements at the front of the vector has to be copied towards the beginning pointer . But why not just changing just the iterator ? How can I avoid these copy ? Thank you for your answer, that was quick ! – mpsido Sep 22 '19 at 11:52
  • See also: https://stackoverflow.com/questions/21310646/stdvectoreraseitem-needs-assignment-operator-to-be-defined-for-item – mpsido Sep 22 '19 at 11:55
  • @mpsido seems you already got the reason why you need a copy assignment for erase :). The rule of 3/5/0 is sth you always need to consider. – Oblivion Sep 22 '19 at 11:59