C++ array of pairs copy constructor and assignment operator

Question

I'm struggling with my copy constructor and assignment operator in my C++ program. I get a segmentation fault (core dump) when testing either of them, individually. I'm building a hash table that is constructed through an array with a pair inside each index. The indexes are chosen based on a hash function, the first part of the pair is a key, the second part of the pair is a value. There's obviously more to the class, but nothing that affects the copy and assignment operator, so I kept them out there. I have no memory leaks and I test the op= and copy constructor with a good number of values already in it.

In UnorderedMap.h

template <typename K, typename V>
class MyUnorderedMap: public Dictionary<K, V>
{
    private:
        MyPair<K, V> *m_data = nullptr; // hash table, array of pairs

        int data_size = 0; // current number of elements inside the array
        int reserved_size = 0; // max elements inside the array

    public:
        // Start data_size and reserved_size at 0, m_data to nullptr
        MyUnorderedMap();

        ~MyUnorderedMap();

        MyUnorderedMap(const MyUnorderedMap<K, V> &source);

        MyUnorderedMap<K, V> & operator=(const MyUnorderedMap<K, V> &source);
}

In UnorderedMap.hpp

// Copy Constructor
template <typename K, typename V>
MyUnorderedMap<K, V>::MyUnorderedMap(const MyUnorderedMap<K, V> &source)
{
  data_size = source.data_size;
  reserved_size = source.reserved_size;
  m_data = new MyPair<K, V>[reserved_size];
  for(int i = 0; i < reserved_size; i++)
  {
    m_data[i].first = source.m_data[i].first;
    m_data[i].second = source.m_data[i].second;
  }
}


// Assignment Operator
template <typename K, typename V>
MyUnorderedMap<K, V> & MyUnorderedMap<K, V>::operator=(const MyUnorderedMap<K, V> &source)
{
  if(this!=&source)
  {
    delete[] m_data;
    reserved_size = source.reserved_size;
    data_size = source.data_size;
    m_data = new  MyPair<K, V>[reserved_size];
    for(int i=0; i<reserved_size; i++)
    {
      m_data[i].first = source.m_data[i].first;
      m_data[i].second = source.m_data[i].second;
    }
  }
  return *this;
}

In MyPair.h

template <typename K, typename V> 
struct MyPair
{
    K first; 
    V second;

    MyPair(){}
    MyPair(const K &key): first(key) {}
    MyPair(const K &key, const V &value): first(key), second(value) {}
};

Does anyone see a problem with why it would be behaving like this? I'm a lot more confident in my copy constructor than the operator=.

Edit x3: I have an insert function not shown that properly inserts into the hash table. So I solved the copy constructor but the op= is still not working.. I fixed the copy constructor above so now it shows a working copy constructor, for anyone else who wants to use it as an effectively working basis. Fixed the assignment operator as well and provided the correct version.

Answer 1

m_data is defined as a pointer in UnorderedMap.h, initialised as nullptr but in the implementation it gets used without being assigned to some actual storage.

Answer 2

I noticed in your copy constructor that you delete [] m_data, but then a few lines later you start assigning values into it; this is a big no-no. After you delete [] it, you have to allocate a new array using m_data = new MyPair<K,V>[data_size]; or something like that.

I also noticed that you initialize your class just in the class declaration, with

private:
    MyPair<K,V>* m_data = nullptr;
    data_size = 0;

and so on. This is generally not how initialization works in C++; you have to put that in the actual constructor, and it doesn't look like you implemented one in your code.

Answer 3

I am assuming that Dictionary has correct copy semantics.

If you take what has been stated from the comments, here is what your code should look like:

First, the copy constructor:

template <typename K, typename V>
MyUnorderedMap<K, V>::MyUnorderedMap(const MyUnorderedMap<K, V> &source) 
        : m_data(new MyPair<K, V>[source.reserved_size]),  // <-- You are missing this
          data_size(source.data_size), 
          reserved_size(source.reserved_size)
{
   for(int i = 0; i < reserved_size; i++)
   {
      m_data[i].first = source.m_data[i].first;
      m_data[i].second = source.m_data[i].second;
   }
}

The copy constructor uses the member-initialization list. Note that the memory is allocated in the member-initialization list. This is the operation you did not have in your version of the copy constructor, and the major issue with your code.

However, there are other issues, which can be addressed with your implementation of the assignment operator.

The next function you should implement after the copy constructor is the destructor, and not the assignment operator. There is a strategic reason why you want to implement the destructor next, to be explained later.

template <typename K, typename V>
MyUnorderedMap<K, V>::~MyUnorderedMap() 
{
   delete [] m_data;
}

Now that you have those two function, the assignment operator becomes trivial. However, let's go over your flawed version:

template <typename K, typename V>
MyUnorderedMap<K, V> & MyUnorderedMap<K, V>::operator=(const MyUnorderedMap<K, V>  
                                                       &source)
{
     if(this!=&source)
     {
         delete[] m_data;
         data_size = source.data_size;

Let's stop right here. You have potentially corrupted your object with those last two lines of code.

The reason is that you've changed the members of your object, and you have yet to allocate memory. What if the call to new[] in the next line throws an exception? You now have an object that is in an invalid state.

If you write the assignment operator this way, the way it should be written is to make sure all the memory is allocated first, and then start to adjust the object's member variables.

But there is a simpler way to avoid all of this:

#include <algorithm>
//...
template <typename K, typename V>
MyUnorderedMap<K, V> & MyUnorderedMap<K, V>::operator=(const MyUnorderedMap<K, V>  
                                                       &source)
{
     if(this!=&source)
     {
         // create a temporary 
         MyUnorderedMap<K,V> temp(source);

         // swap out the temp's contents with the current object
         std::swap(temp.data, data);
         std::swap(temp.data_size, data_size);
         std::swap(temp.reserved_size, reserved_size);

         // temp will be destroyed when the if() goes out of scope
      }
      return *this;
  }

So why does this work? It's simple, and the code basically documents what is done.

You're creating a copy of the passed-in object. Then you're swapping out the contents of the current object's with the copy's contents. Then the copy dies of (after the if block) with the old contents. There is no issue of exceptions being thrown and creating invalid objects, there is no redundant code between the copy constructor and assignment operator, etc.

This technique of writing the assignment operator is called the copy / swap idiom. The only way it works is if you have a correct copy constructor and a correct destructor. That's why we wrote those two functions first, so as to take advantage of simply swap-ping the members in the assignment operator.