C ++ массив пар, конструктор копирования и оператор присваивания

Question

Я борюсь с моим конструктором копирования и оператором присваивания в моей программе на C ++. Я получаю ошибку сегментации (дамп ядра) при тестировании любого из них по отдельности. Я строю таблицу ha sh, которая построена через массив с парой внутри каждого индекса. Индексы выбираются на основе функции ha sh, первая часть пары является ключом, вторая часть пары является значением. Очевидно, что в классе есть что-то еще, но ничего не влияет на операторы копирования и присваивания, поэтому я не стал их скрывать. У меня нет утечек памяти, и я тестирую конструктор op = и copy с уже большим количеством значений.

В 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);
}

В 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;
}

В 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) {}
};

Кто-нибудь видите проблему с тем, почему он будет вести себя так? Я гораздо увереннее в своем конструкторе копирования, чем в операторе =.

Редактировать x3: У меня не показана функция вставки, которая правильно вставляет в таблицу ha sh. Итак, я решил конструктор копирования, но op = все еще не работает. Я исправил конструктор копирования выше, так что теперь он показывает конструктор рабочей копии для всех, кто хочет использовать его в качестве эффективно работающей основы. Также исправлен оператор присваивания и предоставлена ​​правильная версия.

Answer 1

Я заметил в вашем конструкторе копирования, что вы delete [] m_data, но через несколько строк вы начинаете присваивать ему значения; это большой нет-нет. После delete [] этого вы должны выделить новый массив, используя m_data = new MyPair<K,V>[data_size]; или что-то в этом роде.

Я также заметил, что вы инициализируете свой класс только в объявлении класса с помощью

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

и так далее. Это обычно не то, как инициализация работает в C ++; Вы должны поместить это в реальный конструктор, и это не выглядит так, как будто вы реализовали его в своем коде.

Answer 2

Я предполагаю, что Dictionary имеет правильную семантику копирования.

Если вы возьмете то, что было сказано в комментариях, вот как должен выглядеть ваш код:

Сначала конструктор копирования:

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;
   }
}

Конструктор копирования использует список member-initialization. Обратите внимание, что память выделена в списке инициализации элемента. Это операция, которой у вас не было в вашей версии конструктора копирования, и основная проблема с вашим кодом.

Однако есть и другие проблемы, которые можно устранить с помощью вашей реализации оператора присваивания.

Следующая функция, которую вы должны реализовать после конструктора копирования, это деструктор, а не оператор присваивания. Существует стратегическая c причина, по которой вы хотите реализовать деструктор следующим, что будет объяснено позже.

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

Теперь, когда у вас есть эти две функции, оператор присваивания становится тривиальным. Однако давайте go над вашей ошибочной версией:

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;

Давайте остановимся прямо здесь. Вы потенциально испортили свой объект этими двумя последними строками кода.

Причина в том, что вы изменили элементы вашего объекта, и вам еще предстоит выделить память. Что, если вызов new[] в следующей строке вызовет исключение? Теперь у вас есть объект, который находится в недопустимом состоянии.

Если вы пишете оператор присваивания таким образом, он должен быть записан так, чтобы убедиться, что сначала выделена вся память, а затем начать корректировать переменные-члены объекта.

Но есть более простой способ избежать всего этого:

#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;
  }

Так почему же это работает? Это просто, и код в основном документирует, что сделано.

Вы создаете копию переданного объекта. Затем вы заменяете содержимое текущего объекта содержимым копии. Затем копия умирает (после блока if) со старым содержимым. Нет проблем с выдачей исключений и созданием недопустимых объектов, нет избыточного кода между конструктором копирования и оператором присваивания, et c.

Этот метод написания оператора присваивания называется идиома копирования / обмена . Единственный способ это работает, если у вас есть правильный конструктор копирования и правильный деструктор. Вот почему мы сначала написали эти две функции, чтобы воспользоваться преимуществом простого swap -подтверждения членов в операторе присваивания.

Answer 3

m_data определяется как указатель в UnorderedMap.h, инициализируется как nullptr, но в реализации он используется без назначения некоторому фактическому хранилищу.