C++ my copy constructor fails to move the memory to a new area

Question

So I've recently started to work on getting my own vector class working and I'm a little bit stuck on my copy constructor. I'm obviously rather new to c++ and was hoping the good guys at stack overflow could help me out a bit. So I got this copy constructor which copies the actual ptr being used, the end index of the ptr (the elements the user can use) and the actual capacity/reserved memory the ptr has, plus the size of the used memory.

    vector(const vector &other) : storage(other.storage), capacity(other.capacity), 
        endIndex(other.endIndex), m_size(other.m_size)
    {
        T* storage = new T[capacity];
        memcpy(storage, other.storage, sizeof(T) * capacity);
    }

The issue is that while it seemingly copies the information successfully, if one of the objects run out of scope, the information, or at least some of it gets deleted. If I also do a push_back on one of the vector objects, it happens on both of them. So it's fairly safe to say that they share address for their ptr. For example if I run this code in my main function

int main()
{

    vector<int> vec;
    vec.push_back(5);
    vec.push_back(55);
    vec.push_back(500);
    vector<int> vec1 = vec;

    for (int i = 0; i < vec1.size(); i++)
    {
        std::cout << vec1[i] << std::endl;
    }
    return 0;
}

I will get this error message

5
55
500
free(): double free detected in tcache 2
Aborted (core dumped)

I'm assuming this is because the ptr gets deleted while going through the loop and it in turn crashes the program in a nice fashion. Another example of the push_back is

int main()
{

    vector<int> vec;
    vec.push_back(5);
    vec.push_back(55);
    vec.push_back(500);
    vector<int> vec1 = vec;
    vec.push_back(55);

    for (int i = 0; i < vec1.size() + 1; i++)
    {
        std::cout << vec1[i] << std::endl;
    }
    return 0;
}

Where you can obviously see that i actually push_back on the original vector object and not the new one, i even have to increase the for-loops scope to be able to see the object on the new vector, hinting at the size integer in the new object is unchanged from before. Output of this code is:

5
55
500
55
free(): double free detected in tcache 2
Aborted (core dumped)

I don't expect anyone to take time out of their day to debug my code, I don't want that. I'm merely asking for a pair of professional eyes to glance over it and help a newbie out. Thanks in advance.

Answer 1

There are multiple issues with your code.

The first and foremost one is this:

T* storage = new T[capacity];

That storage is not the same as the member variable storage. It is a local variable that just happens to have the same name. Once the copy constructor is complete, you haven't done anything except leak memory.

---

In addition, you have this:

vector(const vector &other) : storage(other.storage),

This assigns the pointer other.storage to this. This is actually where the double-free comes from. You are doing a shallow copy, thus when this and other are destroyed, the same pointer value will be used in the call to delete [] in the destructor.

---

The third issue is this:

memcpy(storage, other.storage, sizeof(T) * capacity);

This will not work for types that are not trivially-copyable. Let's say you fix all the issues except this one. This code will fail miserably:

vector<std::string> s;

and the reason is that you can't use memcpy to copy std::string objects, since std::string is not trivially-copyable.

The fix is to use std::copy, not memcpy, since std::copy is (should be) smart enough to either do a memcpy for trivially-copyable types, or a plain loop for non trivially-copyable types.

---

The last issue is your naming of the class vector. Note that there already is a std::vector in C++. Either change the name to something else, or place your class in its own namespace so that a name clash will not occur if you happen to #include <vector> somewhere.

Putting that all together, you will have this (not compiled, forgive any syntax errors):

---

#include <algorithm>

namespace myVector
{
    template <typename T>
    class vector
    {
       private:
            // your member variables
       public:
         //... 
         vector(const vector &other) : capacity(other.capacity), 
                endIndex(other.endIndex), m_size(other.m_size)
        {
          storage = new T[capacity]();
          std::copy(other.storage, other.storage + other.m_size, storage);
        }

        vector& operator=(const vector& other) 
        {
            // see later
        }

        ~vector()
        {
           delete [] storage;
        } 
      //...
  };
}  
     

Then the main could be this:

#include <myvector>

int main()
{
    myVector::vector<int> vec;
    vec.push_back(5);
    vec.push_back(55);
    vec.push_back(500);
    myVector::vector<int> vec1 = vec;

    for (int i = 0; i < vec1.size(); i++)
    {
        std::cout << vec1[i] << std::endl;
    }
    return 0;
}

Once this is all done and corrected, to complete the rule of 3, the assignment operator can be simply this:

vector& operator=(const vector& other)
{
   if ( &other != this )
   { 
       vector temp(other);
       std::swap(temp.capacity, capacity);
       std::swap(temp.m_size, m_size);
       std::swap(temp.endIndex, endIndex);
       std::swap(temp.storage, storage);
   } 
   return *this;
}

The above is using the copy/swap idiom

Answer 2

The problem is simple, but hard to see in your own code since you know what you want it to do. You could track it down by stepping through in a debugger and examining the value and address of storage scrupulously on every line.

Seriously, try doing that first.

---

OK, so here is it:

vector(const vector &other)
: storage(other.storage)    // 1. copy the pointer, so this->storage is shared
, capacity(other.capacity)
, endIndex(other.endIndex)
, m_size(other.m_size)
{
    // 2. declare a local variable called storage which shadows this->storage
    T* storage = new T[capacity];
    memcpy(storage, other.storage, sizeof(T) * capacity);
}

---

You do not want the storage to be shared, so you should never initialize storage(other.storage). There is literally no reason to do this in your code. If you just initialize it to nullptr you would have realized very quickly that the local variable in the constructor body was wrong.

Simply removing the T* from T* storage = ... will fix your immediate problem. All the other suggestions about using std::copy instead of memcpy, and how better to structure your code are good ones, and you should do those too.