Destructor is being called..

[luckycusp]

Hello

I am facing a problem with this code section

class Triangle
{
public:
 long int ID;
 Normal *origNormal;
 Triangle()
	{ID=0;
        origNormal = new Normal;}
 ~Triangle()
	{
		delete origNormal;
        }        


 /* operator overloading for '=' */
 Triangle operator=(const Triangle & source)
 {
   this->ID = source.ID;
   if(source.origNormal != NULL )
      source.origNormal->CopyTo(this->origNormal);
   
   return *this;
 }
};

class Normal
{
public:
  double nx,ny,nz;
  double angle;
  Normal()
  {
	  nx=0.0,ny=0.0,nz=0.0;  
	  angle = 0.0;  
  }

  void CopyTo(Normal *dest)
  {
	  if(dest!=NULL)
	  {
		  dest->nx = this->nx;
		  dest->ny = this->ny;
		  dest->nz = this->nz;

		  dest->angle = this->angle;
	  }
  }
};

int main()
{
Triangle *CurrTriangle1,*CurrTriangle2;

CurrTriangle1 = new Triangle;
CurrTriangle2 = new Triangle;

/* some arbitrary assignments */
CurrTriangle1->ID = 1;
CurrTriangle1->origNormal->nx = 1.0;

*CurrTriangle2 = *CurrTriangle1;

cout<<"ID of copied Triangle = "<<CurrTriangle2->ID; //ERROR..seems CurrTriangle2 does not exist anymore!!

}

I dont understand why the 2nd triangle is being destroyed somehow!!

[imi]

It shouldn't be destroyed.

But your problem may comes from something different: In line 58 you are accessing "origNormal" which has never been created. Maybe you are overwriting some memory there? I think you are missing an assignment to origNormal in the Triangle constructor.

Ciao, Imi.

[luckycusp]

imi..

Oops sorry, my bad. I edited the code now. Watch the contructor of 'Triangle' class now.

I just did not want to copy my whole code down here, so I just tried copying parts of it here which are important imo. Somehow I missed that line.

By the way, suddenly the codes seems to be working when I made a small change in the overloading function:-

Triangle& operator=(const Triangle & source) //notice the & in return argumument

I wonder why?

[imi]

Hm.. ok, yea.. didn't spot the return-by-value. So what happens is this:

*CurrTriangle2 = *CurrTriangle1;

First, the assignment operator will be called on the object of CurrTriangle2 with the parameter of CurrTriangle1.

But within this assignment operator, the last statement "return *this;" will call the non-existing copy constructor of your Triangle class to copy the *this into a new temporary variable (since you returned by value). Since your copy-constructor is not specified, it generates a default one that just assign all members. So the temporary variable hods a pointer to the same origNormal than *this, which is the CurrTriangle2 - object.

This new and temporary variable is discarded immediately since line 61 don't use the return value. Discarding also means its destructor is called, freeing the same origNormal that CurrTriangle2 is pointing to. Voila.


You can fix this by implementing your own copy constructor. But I rather suggest you think about whether you really need a pointer to the Normal - class or whether a normal non-pointer object will do it. Because then you don't need all the hassle you have at the moment with allocating, destructing, copying around blabla.. the default operator= and default copy-constructor will do just fine.

class Normal // define before Triangle, as now Triangle depends on the complete type
{
public:
  double nx,ny,nz;
  double angle;
  Normal()
  {
	  nx=0.0,ny=0.0,nz=0.0;  
	  angle = 0.0;  
  }
// no CopyToNormal needed
};

class Triangle
{
public:
 long int ID;
 Normal origNormal; // no pointer. Just normal object
 Triangle()
	{ID=0;}
// no destructor needed
// no own operator= needed
// no copy constructor needed
};

Plain, simple, straight-forward.

If you can't live with this (e.g. because you need polymorphism or you don't have the complete type of Normal in the header of Triangle), you can consider using one of the smart pointers like shared_ptr, unique_ptr or auto_ptr.

Ciao, Imi.

[luckycusp]

imi,

Thanks for the answer. But I guess it will take a while for me to understand it!

Regarding using Normal not as pointer, its a long story which I cannot reverse. I got to live with it :(

[imi]

Regarding using Normal not as pointer, its a long story which I cannot reverse. I got to live with it :(

Then implement the copy constructor so that it does a deep copy of origNormal and return a reference in your operator= like you said before.


To add a bit more spice into the topic: You can consider implementing a nothrow-"swap" method which just swaps the pointer of origNormal with the parameter and then implement operator= by using your copy constructor. This is some an idiom for exception-safe implementing operator=. IIRC it was brought up originally by Herb Sutter.

class Triangle
{
...
  void swap(Triangle& rhs) throw() { std::swap(ID, rhs.ID); std::swap(origNormal, rhs.origNormal); }
  Triangle(const Triangle& rhs) : origNormal(new Triangle(rhs.origNormal)), ID(rhs.ID) {}
  Triangle& operator=(const Triangle& rhs) { Triangle tmp(rhs); swap(tmp); return *this; }
...
}

Ciao, Imi.

[luckycusp]

But my CopyTo function in Class Normal does the job for me, so I dont want to m,ake further changes.

I got a new problem now.

void multiply_triangles(Triangle*& start_triangle, int& nTriangles)
{
Triangle *new_ptr=NULL;

new_ptr = new Triangle[nTriangles*5];

delete [] start_triangle; // Here I get an error.. Exeception.. why???

start_triangle = new_ptr;
nTriangles = nTriangles*5;

}

int main()
{
Triangle *ptrTriangle = NULL;
int nTriangles=10;

ptrTriangle = new Triangle[nTriangles];

multiply_triangles(ptrTriangle,nTriangles);

}

[imi]

But my CopyTo function in Class Normal does the job for me, so I dont want to m,ake further changes.

You still need to implement a copy constructor in triangle or you *will* get problems every time you accidently copy an Triangle object.

General rule of thumb: Always implement copy-constructor and operator= together. One without the other leads to trouble.


delete [] start_triangle; // Here I get an error.. Exeception.. why???
What error do you get?

You could use std::vector instead of C-arrays. Then you don't need to bother about destroying the array itself.

std::vector<Triangle> ptrTriangle;
...
ptrTriangle = std::vector<Triangle>(nTriangles);
// no delete[] needed 

Ciao, Imi.

[luckycusp]

General rule of thumb: Always implement copy-constructor and operator= together. One without the other leads to trouble.

I will do that.

Regarding the error, I got, its strange that this time it worked without throwing any exception. I am really scared that the code is very unstable.

Anyway I will look into it further in the evening. Thank you very much for ur help imi.

[moorecm]

Also note that operator= should check that it is not assigning to itself work with self assignment¹.


¹ See below.

[imi]

Also note that operator= should check that it is not assigning to itself.

Well.. I'd say "it should work with self-assignment". Explicit checking for that is one solution, but both versions - luckycusp as well as the exception-safe swap-idiom are both already work with self-assignment.

An explicit check may be faster in those cases where it actually happen a lot.

Ciao, Imi.