Unexpected call of a class destructor

[lalebarde]

Hi all,

I have defined a simple class Tutu with two constructors, the default one that do nothing and a 1 parameter one that allocates and initializes a dynamic array. The destructor frees the memory of course. This code is stupid but just a simple example to show my issue.

Then if I create objects like this : Tutu t(5);, it is alright. But if I do it in two steps : Tutu t; t = Tutu(5); , it fails since the destructor is called for reasons I don't understand.

Please help me understand why the destructor is called. And how can I solve this problem.
BTW, I need the Tutu t; t = Tutu(5); form since in the real case, I declare arrays :
Tutu t[1000]; for (int i = 0; i < 1000; i++) t[i] = Tutu(i);

#include <iostream>
#include <string>
#include <cstdlib>

class Tutu {
public:
	int* mpCommonInts;
	int mQtInts;
	Tutu() {
		cout << "Default constructor for Tutu called" << endl;
	}
	Tutu(int aQtInts) : mQtInts(aQtInts) {
		cout << "Constructor Tutu(" << mQtInts << ") called" << endl;
		mpCommonInts = new int[1000];
		for (int i = 0; i < mQtInts;i++) mpCommonInts[i] = mQtInts * 10000 + i;
	}
	~Tutu() {
		cout << "Destructor ~Tutu called" << endl;
		delete [] mpCommonInts;
	}
};

int main(int argc, const char *argv[]) {
	Tutu t;
	t = Tutu(5);
	cout << "Investigate unexpected call of a Tutu class destructor :";
	for (int i  = 0; i < t.mQtInts; i++) cout << " " << t.mpCommonInts[i];
	cout << endl;
	return 0;
}

$ ./Release/test
Default constructor for Tutu called
Constructor Tutu(5) called
Destructor ~Tutu called
Investigate unexpected call of a Tutu class destructor : 50000 50001 50002 50003 50004
Destructor ~Tutu called
*** glibc detected *** ./Release/test: double free or corruption (top): 0x0000000001a6b140 ***

[coder777]

The reason is that on line 29 you copy the pointer. So you have 2 objects the contain the pointer to the same thing. both trying to destroy that thing in their destructor which will cause the crash

[Gaminic]

The destructor frees the memory of course.

There is nothing of course about it. You have an empty constructor as well, which means that an object can exist that doesn't have any "new'ed" memory allocation. What if I do

Tutu t = new Tutu;
delete t;

Now, the destructor is called, which calls delete on the allocated memory in t. But wait! We didn't allocate any memory! Aaagh!

Either do a simple check before deleting:
if (myVariables != nullptr) delete[] myVariables;
Or make a "cleaning" function that deletes all allocated memory, but only if YOU explicitly say so.

[lalebarde]

Thanks both of you.

Gaminic : Good point but not enough. Here is the new code and the result :

class Tutu {
public:
	int* mpCommonInts;
	int mQtInts;
	Tutu() {
		mpCommonInts = NULL;
		cout << "Default constructor for Tutu called, mpCommonInts = " << mpCommonInts << endl;
	}
	Tutu(int aQtInts) : mQtInts(aQtInts) {
		mpCommonInts = new int[mQtInts];
		cout << "Constructor Tutu(" << mQtInts << ") called, mpCommonInts = " << mpCommonInts << endl;
		for (int i = 0; i < mQtInts;i++) mpCommonInts[i] = mQtInts * 10000 + i;
	}
	~Tutu() {
		cout << "Destructor ~Tutu called, mpCommonInts = " << mpCommonInts << endl;
		if (mpCommonInts != NULL) delete [] mpCommonInts;
	}
};

$ ./Release/test
Default constructor for Tutu called, mpCommonInts = 0
Constructor Tutu(5) called, mpCommonInts = 0x1d4d140
Destructor ~Tutu called, mpCommonInts = 0x1d4d140
Investigate unexpected call of a Tutu class destructor : 0 0 50002 50003 50004
Destructor ~Tutu called, mpCommonInts = 0x1d4d140
*** glibc detected *** ./Release/test: double free or corruption (fasttop): 0x0000000001d4d140 ***

coder777 : if I understand well, I delete an array already deleted. The point is that I don't understand why the destructor for

mpCommonInts = 0x1d4d140

is called before the cout. I would have expected the destructor for

mpCommonInts = 0

at this stage.

[coder777]

The point is that I don't understand why the destructor for

mpCommonInts = 0x1d4d140

is called before the cout.

Well, on line 29 the object right of the assignment operator is created 'on the fly'. It's life ends right after the assignment is done and hence before the 'cout' and so is the destructor called

[lalebarde]

coder777 : thanks for the explanation.

I have tried to add a copy constructor. But it is not called and the output is unchanged !

	Tutu(const Tutu& a)  {
		mQtInts = a.mQtInts;
		mpCommonInts = a.mpCommonInts;
		cout << "Copy constructor of Tutu. Arg.mpCommonInts = " << a.mQtInts << endl;
	}

So, what is the good way ?

[Peter87]

You should also define a copy assignment operator
See rule of three: http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29

[lalebarde]

Thanks Peter87. So I have added a copy assignment operator :

	Tutu& operator=(const Tutu& a) {
		mQtInts = a.mQtInts;
		mpCommonInts = a.mpCommonInts;
		cout << "operator= of Tutu. mpCommonInts = " << mpCommonInts << endl;
		return (*this);
	}

Here is the new output :

$ ./Release/test
Default constructor for Tutu called, mpCommonInts = 0
Constructor Tutu(5) called, mpCommonInts = 0x6e0140
operator= of Tutu. mpCommonInts = 0x6e0140
Destructor ~Tutu called, mpCommonInts = 0x6e0140
Investigate unexpected call of a Tutu class destructor : 0 0 50002 50003 50004
Destructor ~Tutu called, mpCommonInts = 0x6e0140
*** glibc detected *** ./Release/test: double free or corruption (fasttop): 0x00000000006e0140 ***

Here I understand the error since I defined operator= to keep the same pointer value and then I try to delete the array twice.

The point is that I don't want the data to be copied because in the real case that's very big. How could I do ?

[Peter87]

Why don't you just create t as Tutu t(5); if you don't want copying?

If you really don't want copy constructor and assignment operator to copy the data you would have to have some kind of reference counter to keep track of how may objects that are using the data. If you the data can be changed you probably also want to implement copy on write, that is if the reference counter is greater than 1, copy the data, decrement the reference counter and make the changes to the copied data. This becomes a bit complicated so I doubt it is worth it.

If you decide that copying is fine after all, I would recommend that you used a std::vector to store the data. Then you don't need to define the copy constructor, copy assignment operator or destructor because the defaults are enough.

[lalebarde]

Thanks Peter87. That helps.

Why don't you just create t as Tutu t(5); if you don't want copying?

Because in the real case, I declare arrays (huge ones) :

Tutu t[10000000]; for (int i = 0; i < 10000000; i++) t[i] = Tutu(i);

and I cannot declare them just like with Tutu t(5);.

Due to their size, I have to forbid copies.

[lalebarde]

Ok, this is a question of paradigm. My conclusion is that for this kind of class, a specific paradigm is required. Here is what it has lead me to:

For dynamic attributes, delay their creation till object is assigned

class Tutu2 {
public:
	int* mpCommonInts;
	int mQtInts;
	Tutu2() {
		mpCommonInts = NULL;
		cout << "Default constructor for Tutu2 called, mpCommonInts = " << mpCommonInts << endl;
	}
	Tutu2(int aQtInts) : mQtInts(aQtInts) {
		mpCommonInts = NULL;
		cout << "Constructor Tutu2(" << mQtInts << ") called, mpCommonInts = " << mpCommonInts << endl;
	}
	Tutu2(const Tutu2& a)  { //: mQtInts(a.mQtInts), mpCommonInts(a.mpCommonInts)
		mQtInts = a.mQtInts;
		mpCommonInts = new int[mQtInts];
		for (int i = 0; i < mQtInts;i++) mpCommonInts[i] = a.mpCommonInts[i];
		cout << "Copy constructor of Tutu2. mpCommonInts = " << mpCommonInts << endl;
	}
	Tutu2& operator=(const Tutu2& a) {
		// if dest exists, check for size compatibility and reinitialize if required
		if (mpCommonInts != NULL && mQtInts != a.mQtInts) {
			delete [] mpCommonInts;
			mpCommonInts = NULL;
		}
		// copy non dynamically allocated attributes
		mQtInts = a.mQtInts;
		if (mpCommonInts == NULL) mpCommonInts = new int[mQtInts];
		if (a.mpCommonInts == NULL) { // nothing from the source - shall initialize itself
			for (int i = 0; i < mQtInts;i++) mpCommonInts[i] = mQtInts * 10000 + i;
		} else { // source data exists => just copy it
			for (int i = 0; i < mQtInts;i++) mpCommonInts[i] = a.mpCommonInts[i];
		}
		cout << "operator= of Tutu2. mpCommonInts = " << mpCommonInts << endl;
		return (*this);
	}
	~Tutu2() {
		cout << "Destructor ~Tutu2 called, mpCommonInts = " << mpCommonInts << endl;
		if (mpCommonInts != NULL) delete [] mpCommonInts;
	}
};


int main(int argc, const char *argv[]) {
	Tutu2 t, u;
	t = Tutu2(5);
	cout << "t = Tutu2(5) = "; for (int i  = 0; i < t.mQtInts; i++) cout << " " << t.mpCommonInts[i]; cout << endl;
	t = Tutu2(6);
	cout << "t = Tutu2(6) = "; for (int i  = 0; i < t.mQtInts; i++) cout << " " << t.mpCommonInts[i]; cout << endl;
	u = t;
	cout << "u = t = "; for (int i  = 0; i < u.mQtInts; i++) cout << " " << u.mpCommonInts[i]; cout << endl;
	return 0;
}

$ ./Release/test
Default constructor for Tutu2 called, mpCommonInts = 0
Default constructor for Tutu2 called, mpCommonInts = 0
Constructor Tutu2(5) called, mpCommonInts = 0
operator= of Tutu2. mpCommonInts = 0x1f4b140
Destructor ~Tutu2 called, mpCommonInts = 0
t = Tutu2(5) =  50000 50001 50002 50003 50004
Constructor Tutu2(6) called, mpCommonInts = 0
operator= of Tutu2. mpCommonInts = 0x1f4b140
Destructor ~Tutu2 called, mpCommonInts = 0
t = Tutu2(6) =  60000 60001 60002 60003 60004 60005
operator= of Tutu2. mpCommonInts = 0x1f4b160
u = t =  60000 60001 60002 60003 60004 60005
Destructor ~Tutu2 called, mpCommonInts = 0x1f4b160
Destructor ~Tutu2 called, mpCommonInts = 0x1f4b140

Now it works just fine ! Thanks all again for your help. That has made me progress in my C++ understanding.

[coder777]

Investigate unexpected call of a Tutu2 class destructor : 50000 50001 50002 50003 50004

That doesn't sound overly fine... Where does this message come from?

I'd suggest using smart pointer instead

[lalebarde]

This message was only to make the code informative when the issue did exist in the first post. As you can see in the last code, I replaced it (possibly you saw my last post before I last EDIT it. Sorry).
With huge data (several tables of millions of struct), I prefer master precisely when things are done.

[cire]

Well, that's not much of a solution. You've basically said "I need to use an init function after creating my tutu to get it into the right state, and I'm co-opting operator= for that purpose."

Very fragile.

int main()
{
       tutu2 t = tutu2(5) ;
       tutu2 u = tutu2(6) ;

	cout << "t = Tutu2(5) = "; for (int i  = 0; i < t.mQtInts; i++) cout << " " << t.mpCommonInts[i]; cout << endl;
	cout << "u = Tutu2(6) = "; for (int i  = 0; i < u.mQtInts; i++) cout << " " << u.mpCommonInts[i]; cout << endl;
}

If that doesn't result in a program crash, I will be terribly surprised.

If you don't wish to allow copying, don't allow copying. Make your copy constructor and assignment operator private and don't implement them. Or, use the c++11 way with "= delete" if your compiler supports it.

[Peter87]

If you are using C++11 you can make a move constructor and move assignment operator and disable the normal copy constructor and copy assignment operator (as cire mentioned). That way you can do

Tutu t = Tutu(2);
t = Tutu(5);

without problem (no copying) but things like

Tutu t1;
Tutu t2 = t1;

will fail to compile.