Failure to delete pointer in destructor

Question

I ran into a lack of deep understanding of C ++ pointers. I wrote a class called Skymap, which has the following definition:

class Skymap { public: Skymap(); ~Skymap(); void DrawAitoffSkymap(); private: TCanvas mCanvas; TBox* mSkymapBorderBox; }; 

with functions defined as:

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

(I use the ROOT build package, but I think this is just a general C ++ question).

Now the following program will crash when the skymap2 destructor is reached:

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

However, the following will not crash:

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

Also, if I initialize the mSkymapBorderBox pointer to NULL in the constructor, I no longer crash after executing the first program (with two Skymap objects).

Can someone explain what is the main reason for this? There seems to be a problem with the pointer in the second Skymap object, but I don't see what it is.

Answer 1

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

Here you allocate the memory of a local variable, not a member variable. And since you do not allocate memory for a member variable, calling delete on it causes a call to undefined, which leads to failure in your case.

What you should do is:

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

which now allocates memory for a member variable. This is one of the reasons why local variables should be named differently than member variables. Naming conventions help to avoid such errors.

As a side element, or rather a very important note, since your class manages resources, consider implementing the semantics of semantics with the destructor properly: this rule is generally called the-rule-of-three . Or use some smart pointers such as std::shared_ptr , std::unique_ptr or whatever suits your scenario.

Answer 2

Nawaz's answer is correct. But besides this, your code has several possible problems:

• If someone creates SkyMap and never calls DrawAitoffSkymap with it, you will get undefined behavior (since mSkymapBorderBox is never initialized, it will have a random value, which you then delete).
• If someone calls DrawAitoffSkymap more than once using this SkyMap, you will get a memory leak.

To fix it:

(1) Initialize mSkymapBorderBox to zero in your constructor.

(2) Determine what DrawAitoffSkymap should do if it called several times. If it should reuse the old mSkymapBorderBox, then you would like to say something like:

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

On the other hand, if a new TBox has to be created every time, then you want:

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

Answer 3

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100); you create a new TBox* pointer that is not a data member.

Think about the correct implementation of delete after implementing new in the same logical block / scope ...

Answer 4

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

when you declared this, create an object of class TBox. When you exit DrawAitoffSkymap at this time will lose the link to the allocated memory.

When the destructor is called during, it frees up some garbage memory.

To avoid this, use this mSkymapBorderBox=new TBox(-200,-100,200,100);
instead of TBox* mSkymapBorderBox=new TBox(-200,-100,200,100);