Двойная бесплатная ошибка

Question

Я сделал функцию для объекта с именем copy (), которая должна просто возвращать экземпляр объекта со всеми одинаковыми значениями -

Grid Grid::copy() {

Grid result;

result.setFilename(f_name);
result.setNumOfRows(num_rows);
result.setNumOfCols(num_cols);
result.setMap(map);


return result;
}

Мой деструктор выглядит так -

Grid::~Grid() {
for(int r=0;r<num_rows;r++)
    delete [] map[r];
}  

Теперь, когда мой код работает и вызывается функция копирования, я получаю ошибку

*** glibc detected *** ./go: double free or corruption (!prev): 0x0982c6a8 ***

с большим количеством другой информации (большая стена текста) после этого.Это просто означает, что память удаляется дважды правильно?Если так, как это может быть?Почему деструктор вызывается дважды?

Код, по которому он вызывается, выглядит так:

for(;;) {
    Grid g;

    if(which_display == 1) {

       .....
       .....
        g = myServer->getAgent()->getGrid()->copy(); //HERE


    }
    //print
    std::cout<<g.toString();
}

Мне кажется, что я упускаю что-то очевидное.Может кто-нибудь указать мне, как деструктор вызывается дважды?

Answer 1

Ваша функция копирования не создает глубокую копию карты;это просто копирование карты указателей.Когда деструктор вызывается для исходного объекта и копии, эти указатели удаляются дважды.

Answer 2

Вам не хватает конструктора копирования и оператора присваивания. Это Закон Большой Тройки .

Большая тройка:

1. конструктор копирования
2. оператор присваивания
3. деструктор

Закон большой тройки таков: если вам нужен один из них, есть хороший шанс, что вам нужны все три. Обычно они связаны с обработкой ресурсов нетривиальным образом.

В вашем примере вы явно освобождаете память в деструкторе. Это, вероятно, означает, что вам нужно обрабатывать память специально в конструкторе копирования и операторе присваивания: либо правильно распределить новую память и скопировать значения, либо заблокировать копирование и присвоение (объявив их закрытыми и не реализовав их).

Answer 3

Вы возвращаете временный объект из функции copy. Вероятно, вам нужно выделить Grid в куче, а затем передать указатель (или, что лучше, умный указатель):

Grid *Grid::copy() {

Grid *result = new Grid();

result->setFilename(f_name);
result->setNumOfRows(num_rows);
result->setNumOfCols(num_cols);
result->setMap(map);


return result;
}

Версия с умным указателем (вы также можете использовать std::shared_ptr с C ++ 11):

boost::shared_ptr<Grid> Grid::copy() {

boost::shared_ptr<Grid> result(new Grid());

result->setFilename(f_name);
result->setNumOfRows(num_rows);
result->setNumOfCols(num_cols);
result->setMap(map);


return result;
}

В опубликованном вами коде результат уничтожается при выходе из функции, и вы получаете неопределенное поведение.

РЕДАКТИРОВАТЬ: Также убедитесь, что глубоко скопировали карту, как указано в комментариях Чада. В качестве альтернативы вы также можете использовать shared_ptr для экономии затрат на копирование.

Answer 4

Вы на самом деле не хотите copy метод вообще. Вам просто нужен конструктор копирования и оператор присваивания. Я предполагаю, что ваша строка изначально выглядела так:

g = myServer->getAgent()->getGrid();

И так как это не сработало, вы добавили метод копирования. Но в настоящее время исправление только одного метода копирования не решит проблему, так как вам также понадобятся конструктор копирования и оператор присваивания, или ваша тяжелая работа по исправлению метода копирования может быть уничтожена.

Во-первых, краткое объяснение того, что происходит и почему программа не работает:

1. Звоните copy
2. Это вводит ваш метод копирования, который создает Grid в стеке.
3. Он устанавливает членов Grid, но мы подозреваем, что это мелкая копия.
4. Возвращает метод copy, который вызывает конструктор копирования Grid. *
5. Конструктор копирования по умолчанию делает поверхностную копию.
6. Деструктор Grid на основе стека срабатывает, удаляя содержимое map.
7. Метод копирования теперь вернулся, предоставив временную Grid, но такую, которая указывает на удаленную память.
8. Теперь временный объект Grid назначен на g. Это вызывает оператор присваивания Grid. *
9. Оператор присваивания по умолчанию выполняет поверхностное копирование, как это делал конструктор копирования по умолчанию.
10. В конце строки временный объект уничтожается, где он пытается удалить содержимое map, которое уже было удалено. стрела.
11. Когда g выходит из области видимости, его деструктор снова попытается удалить содержимое map.

Как видите, есть 3 места, где происходит мелкое копирование - все должно быть исправлено, иначе это все равно не получится.

Как это исправить

1. Избавьтесь от метода копирования - он все равно не дает никакого значения.
2. Исправьте setMap и setFilename, чтобы сделать глубокую копию.
3. Создать оператор присваивания. Это должно глубоко копировать содержимое другой сетки.
4. Создайте конструктор копирования, как оператор присваивания.

Вот как может выглядеть оператор присваивания (при условии, что все заданные методы выполняют глубокое копирование):

Grid& operator= (const Grid& g) {
  setFilename(f_name);
  setNumOfRows(num_rows);
  setNumOfCols(num_cols);
  setMap(map);

  return *this;
}

Существуют методы написания конструктора копирования, а затем заставить оператор присваивания использовать конструктор копирования. Это хорошая техника (меньше дублированного кода), но у меня нет удобной ссылки. Когда я найду это, я свяжу это здесь.

Наконец, я отметил несколько строк в своем объяснении (*). Компиляторы могут выполнять оптимизацию возвращаемых значений (RVO) и именованные RVO. С этими оптимизациями он фактически не будет создавать объект Grid в стеке в пределах copy, а затем копировать-конструкцию для возвращаемого значения - он просто создаст временный объект для результата copy, а затем метод copy будет использовать его вместо своего собственного Grid объекта, основанного на стеке. Таким образом, при достаточном количестве оптимизаций компилятора ваш код может преодолеть эту точку и позже потерпеть крах. Очевидно, это бесполезно, так что это, скорее, к сведению.

Answer 5

В опубликованном вами коде отсутствуют важные части, такие как определение класса и реализация setMap. Nonetheles, из того, что я увидел, я могу сделать вывод, что, вероятно, проблема возникает, когда компилятор вызывает конструктор копирования по умолчанию для временного объекта (возвращаемое значение). В итоге вы получите два объекта Grid, чей член карты указывает на одну и ту же память, которая была выделена только один раз, и, очевидно, их соответствующие вызовы деструктора будут конфликтовать. Если у вас есть какой-либо динамически распределенный член в вашем классе (как, например, карта), вы должны сделать одну из следующих вещей:

a) Полностью поддерживает семантику значений, реализует конструктор по умолчанию, конструктор копирования и оператор присваивания б) Запретите это, объявив (а не определив) конструктор копирования и оператор присваивания частным.

Вариант b) является предпочтительным выбором, если ваш объект требует больших затрат (в памяти и времени выполнения).

Если вы выберете (а), вам не нужен метод copy (), просто выполните задание. Если вы выберете (b), ваш метод copy () должен вернуть указатель (предпочтительно умный указатель), как показано на примере dark_charlie. В этом случае я бы также предложил переименовать copy () в clone (), что является наиболее популярным соглашением для имени такого метода.