0

I have the class below and tried to add copy and move constructor and assignment operator. My goal is to have least amount of copy and be as optimized as possible.

I expect the vectors to be filled in place, that is no copy constructors be called while creating the vector. What am I doing wrong and how to force it to use move constructor or assignment?

#include <iostream>
#include <concepts>
#include <vector>

template<typename T>
requires std::is_arithmetic_v<T>
class Data {
    private :
    T mA = 0;
    T mB = 0;
    
    public: 
    Data(const T& data) : mA{data}{ // from single T
        std::cout << "Constructed Data with: " << mA << ", " << mB << std::endl;
    }

    Data(const Data<T>& other)  : mA{other.mA}, mB{other.mB} {
        std::cout << "COPY Constructed Data with: " << mA << ", " << mB << std::endl;
    }
     
    Data(Data<T>&& other)  : mA{other.mA}, mB{other.mB} {
        std::cout << "MOVE Constructed Data with: " << mA << ", " << mB << std::endl;
    }    

    Data(const std::initializer_list<T>& list) {
        std::cout << "Constructed Data with list: "; 
        if(list.size() >= 2) {
            mA = *list.begin();
            mB = *(list.begin() + 1);
            std:: cout << mA << ", " << mB << std::endl;
        }
    }

    ~Data() {
        std::cout << "Destructed: " << mA << ", " << mB << std::endl;
    }

    const Data operator=(const Data& other) {
        mA = other.mA;
        mB = other.mB;
        return *this;
    }

    Data operator=(Data&& other) {
        mA = other.mA;
        mB = other.mB;
        return *this;
    }
};

int main() {

    std::cout << "** With initilizer_list **" << std::endl;
    {
        auto vec = std::vector<Data<int>>{{1,1}, {2,2}};
    }


    std::cout << "\n**With element**" << std::endl;
    {
        auto vec = std::vector<Data<int>>{1,2};
    }

   std::cout << "\n**With push**" << std::endl;
   {
       auto vec = std::vector<Data<int>>();
       vec.push_back(1);
       vec.push_back(2);
   }

}

Output:

** With initilizer_list **
Constructed Data with list: 1, 1
Constructed Data with list: 2, 2
COPY Constructed Data with: 1, 1
COPY Constructed Data with: 2, 2
Destructed: 2, 2
Destructed: 1, 1
Destructed: 1, 1
Destructed: 2, 2

**With element**
Constructed Data with: 1, 0
Constructed Data with: 2, 0
COPY Constructed Data with: 1, 0
COPY Constructed Data with: 2, 0
Destructed: 2, 0
Destructed: 1, 0
Destructed: 1, 0
Destructed: 2, 0

**With push**
Constructed Data with: 1, 0
MOVE Constructed Data with: 1, 0
Destructed: 1, 0
Constructed Data with: 2, 0
MOVE Constructed Data with: 2, 0
COPY Constructed Data with: 1, 0
Destructed: 1, 0
Destructed: 2, 0
Destructed: 1, 0
Destructed: 2, 0

CompilerExplorer Link

DEKKER
  • 877
  • 6
  • 19
  • 1
    That single copy constructor call on `push_back` is caused by the missing `noexcept` on the move constructor and move assignment. The remaining copies are caused by the fact that `initializer_list` (which `std::vector` uses as its constructor parameter) can only store const elements, so they have to be copied out of it. If you want zero copies/moves when creating a vector, you need a `reserve` followed by `emplace_back`s. You can't do it with vector's constructor (unless you write some fancy custom iterators). – HolyBlackCat Dec 29 '21 at 22:07
  • I'd replace the `initializer_list` constructor with a constructor with two args Then you wouldn't have to copy the members from the list, and there would be no risk of getting an incorrectly sized list. *"tried to add copy and move constructor and assignment ... goal is to have least amount of copy and be as optimized as possible"* Your custom copy/move constructors/assignments aren't going to be better than the ones generated by the compiler. But they can be worse if you write them wrong (missing `noexcept` on move operations, missing `std::move` for fields in the move operations). – HolyBlackCat Dec 29 '21 at 22:12

1 Answers1

2

I expect the vectors to be filled in place

Both push_back and the initializer list constructor expect that the element is already constructed and passed through the parameter. Therefore the elements must first be constructed via conversion and then moved/copied into the vector's storage. If you don't want that, then use emplace_back instead, which takes constructor arguments for the element's constructor and creates the element in-place.

This doesn't resolve all cases of copy and moves, since the vector must move objects to new storage if the old one becomes too small to hold more elements. To avoid this completely, first call vec.reserve(...) where ... is at least as large as the maximum size that the vector will have.

The reason copy instead of move is used in case of reallocation is because you didn't mark your move constructor noexcept. std::vector prefers the copy constructor if the move constructor is not noexcept, because if a move constructor throws while moving the objects to new storage, then std::vector cannot guarantee that it will be able to roll-back to the previous state, as it would usually do.


Declaring copy/move constructor/assignment or a destructor in a class is always a bit risky. Firstly, because how easy it is to cause undefined behavior, see rule of three/five and secondly because it often results in worse results than the implicitly generated versions if not carefully written.

If you don't have a specific reason, e.g. because you manage a raw resource in the class, then don't declare any of these special members. (Only) Then the implicit ones will do the correct and most-likely best possible thing automatically ("rule of zero").

user17732522
  • 53,019
  • 2
  • 56
  • 105