Wheels2050 Wheels2050 - 3 months ago 14
C++ Question

Crash When Deleting Pointer in Destructor

I've bumped up against my lack of deep understanding of pointers in C++. I've written a class called Skymap, which has the following definition:

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

TBox* mSkymapBorderBox;
};


with the 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 am using the ROOT plotting package, but I think this is just a general C++ question).

Now, the following program will crash upon reaching the destructor of skymap2:

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 initialise the pointer mSkymapBorderBox to NULL in the constructor, I no longer experience a crash following the execution of the first program (with 2 Skymap objects).

Can anyone please explain what the underlying cause of this is? It appears to be a problem with the pointer in the second Skymap object, but I do not see what it is.

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

Here you're allocating memory to a local variable, not to the member variable. And since you're not allocating memory for the member variable, calling delete on it would invoke undefined behavior, which results in crash in your case.

What you should be doing is this:

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

which now allocates memory for the member variable. It is one of the reason why local variables should be named differently than member variables. Naming conventions helps avoiding such bugs.

As a sidenote, or rather a very important note, since your class manages resources, consider implementing copy-semantics along with destructor properly: this rule is popularly referred to as the-rule-of-three. Or use some smart pointers, such asstd::shared_ptr, std::unique_ptr or whatever fits in your scenario.

Comments