Ошибка при удалении указателя в деструкторе

Question

Я столкнулся с недостатком глубокого понимания указателей на C++. Я написал класс под названием Skymap, который имеет следующее определение:

class Skymap { 
public: 
    Skymap(); 
    ~Skymap(); 
    void DrawAitoffSkymap(); 
private: 
    TCanvas mCanvas; 

    TBox* mSkymapBorderBox; 
}; 

с функциями, определенными как:

#include "Skymap.h" 

Skymap::Skymap() 
{ 
    mCanvas.SetCanvasSize(1200,800); 
    mMarkerType=1; 
} 

Skymap::~Skymap() 
{ 
    delete mSkymapBorderBox; 
} 

void Skymap::DrawAitoffSkymap() 
{ 
    TBox* mSkymapBorderBox=new TBox(-200,-100,200,100); 
    //Use the mSkymapBorderBox pointer for a while 
} 

(я использую пакет построения графиков ROOT, но я думаю, что это просто общий вопрос на C++).

Теперь, следующая программа будет сбой при достижении деструктора skymap2:

int main(){ 
    Skymap skymap1; 
    Skymap skymap2; 
    skymap1.DrawAitoffSkymap(); 
    skymap2.DrawAitoffSkymap(); 
    return(0); 
} 

Однако следующая не буду авария:

int main(){ 
    Skymap skymap1; 
    skymap1.DrawAitoffSkymap(); 
    return(0); 
} 

Кроме того, если я инициализирую указатель mSkymapBorderBox на NULL в конструкторе я больше не испытываю сбой после выполнения первой программы (с двумя объектами Skymap).

Может ли кто-нибудь объяснить причину этого? Это проблема с указателем во втором объекте Skymap, но я не вижу, что это такое.

Answer 1

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100); 

Здесь вы назначаете память локальной переменной, а не членной переменной. И поскольку вы не выделяете память для переменной-члена, вызов delete на ней вызывает неопределенное поведение, что приводит к сбою в вашем случае.

То, что вы должны делать это:

mSkymapBorderBox=new TBox(-200,-100,200,100); 

который теперь выделяет память для переменной-члена. Это одна из причин, почему локальные переменные должны быть названы иначе, чем переменные-члены. Соглашения об именах помогают избежать таких ошибок.

Как заметка на полях, или, вернее, очень важное замечание, так как ваш класс управляет ресурсов, рассмотреть вопрос об осуществлении копирования-семантику вместе с деструктора должным образом: это правило в народе называют the-rule-of-three. Или используйте некоторые интеллектуальные указатели, такие как std::shared_ptr, std::unique_ptr или все, что подходит вашему сценарию.

Answer 2

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

1. Если кто-то создает SkyMap и никогда не называет DrawAitoffSkymap с ним, то вы получите неопределенное поведение (так как mSkymapBorderBox никогда не инициализируется, он будет иметь случайное значение, который вы затем удалите).
2. Если кто-то называет DrawAitoffSkymap более одного раза с помощью данного SkyMap, вы получите утечку памяти.

Чтобы исправить:

(1) Initialize mSkymapBorderBox к нулю в конструкторе.

(2) Определите, что должен сделать DrawAitoffSkymap, если он называется несколько раз.Если он должен повторно использовать старый mSkymapBorderBox, то вы хотели бы сказать что-то вроде:

void Skymap::DrawAitoffSkymap() { 
    if (!mSkymapBorderBox) mSkymapBorderBox = new TBox(...); 
    ... 
} 

С другой стороны, если новый TBox должен быть создан каждый раз, то вы хотите:

void Skymap::DrawAitoffSkymap() { 
    delete mSkymapBorderBox; // note: does nothing if mSkymapBorderBox == 0 
    mSkymapBorderBox = new TBox(...); 
    ... 
} 

Answer 3

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100); вы создаете новый указатель TBox*, который не является членом данных.

Рассмотрим правильно реализовать delete после реализации new, в одной и той же логической единицы/объем ...

Answer 4

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100); 

когда вы объявили это это создать объект класса TBox. Когда вы выходите из DrawAitoffSkymap, в это время это потеряет ссылку на эту выделенную память.

Когда деструктор называется вовремя, он освобождает некоторую память мусора.

Чтобы избежать этого использовать этот
mSkymapBorderBox=new TBox(-200,-100,200,100);
вместо TBox* mSkymapBorderBox=new TBox(-200,-100,200,100);