CODEWITHSUNDEEP
c++pointersdestructor
Wheels2050
Wheels2050

Reputation: 889

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.

Upvotes: 4

Views: 11560

Answers (4)

Prasanta
Prasanta

Reputation: 1

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

when you declared this this create an object of the class TBox. When you exit DrawAitoffSkymap at that time this lose the reference of this allocated memory.

When the destructor is called at time it frees 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);

Upvotes: 0

kiriloff
kiriloff

Reputation: 26335

TBox* mSkymapBorderBox=new TBox(-200,-100,200,100); you are creating a new TBox* pointer, which is not the data member.

Consider to properly implement the delete after you implement a new, in the same logical unit/scope...

Upvotes: 0

Sarfaraz Nawaz
Sarfaraz Nawaz

Reputation: 361292

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.

Upvotes: 16

Edward Loper
Edward Loper

Reputation: 15944

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

  1. If anyone creates a SkyMap and never calls DrawAitoffSkymap with it, then you'll get undefined behavior (since mSkymapBorderBox is never initialized, it will have a random value, which you then delete).
  2. If anyone calls DrawAitoffSkymap more than once with a given SkyMap, then you'll get a memory leak.

To fix:

(1) Initialize mSkymapBorderBox to zero in your constructor.

(2) Decide what DrawAitoffSkymap should do if it's called multiple times. If it should reuse the old mSkymapBorderBox, then you would want to say something like:

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

On the other hand, if a new TBox should be created each time, then you want:

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

Upvotes: 7

Related Questions