Safely deleting a class instance

DividedByZero · 2012-08-01T09:48:17

Hi Guys, I have a vector of classes. How do I safely delete an item in the vector so the destructor gets called. At the moment I have the following. But I suspect this is a 'brute force' removal. How do I call 'delete' properly? for(it=VectorAsset.begin();it<VectorAsset.end();it++) { if(szNameOrType==it->szName||szNameOrType==it->szType) { //delete it; // <<< wont compile VectorAsset.erase(it); } } Any help would be awesome

For Beginners

Started by DividedByZero July 31, 2012 06:20 AM

14 comments, last by Tallkotten 11 years, 8 months ago

BitMaster

8,652

July 31, 2012 08:49 AM

Whenever you insert something into the vector with (for example) myVector = MyClass(...) or myVector.push_back(MyClass(...)) the following things happen:
1) an unnamed instance (let's still call it U to identify it) of MyClass is constructed.
2) a copy constructor (or assignment operator) of the appropriate element in the vector is called for U.
3) U gets destroyed.
Furthermore, whenever you insert or remove elements from the vector, the vector might have to reallocate/reorder its elements, leading to further copy-or-assignment/destructor calls.

DividedByZero

1,261

Author

July 31, 2012 08:55 AM

So, should I ignore the destructors and call a function within the class to handle the cleanup instead?

rip-off

11,000

July 31, 2012 09:02 AM

Your loop is incorrect. It allows invalidated iterators to be accessed. It should be:



void SpriteUnload(string szNameOrType)

{

	// Erase any valid items from vector array

	std::vector<Asset>::iterator it = VectorAsset.begin();

	while(it != VectorAsset.end())

	{

		if(szNameOrType==it->szName || szNameOrType==it->szType || szNameOrType=="ALL")

		{

			delete *it;

			it = VectorAsset.erase(it);

		}

		else

		{

			++it;

		}

	}

}

Note the assignment to "it" of the return value of "erase()".

std::vector<class Asset>(VectorAsset);
[/quote]
The "class" keyword here is superfluous. It doesn't change anything. Putting the variable's name in parenthesis is highly unusual and degrades readability.

So, when I call VectorAsset.erase(it); does the destructor get called still?
[/quote]
Some destructor will get called, yes. That is necessary, but not sufficient, for ensuring you are not leaking. The real question I suspect you want to know the answer to is whether you are leaking memory.

As I have alot of DirectX code that needs to be cleaned up via the destructor (in each instance).
[/quote]
Are you correctly implementing a copy constructor? If not, you cannot store your objects in the vector by value. As the others have mentioned, you almost certainly want to store (smart) pointers to your objects in the vector.

Testing my destructor, it is actually getting called when the class is created.
[/quote]
When you are copying the object into the vector, a temporary is created. This temporary is then destroyed. If you log your constructor, copy constructor, assignment operator and destructor you'll get a better picture of the lifecycle of various instances (particularly if you print the value of "this" in each log statement).

So, should I ignore the destructors and call a function within the class to handle the cleanup instead?
[/quote]
No, you need to understand C++ value and reference semantics, and choose the appropriate one. If you choose value semantics, you must also correctly implement the rule of three.

blog | twitter

BitMaster

8,652

July 31, 2012 09:06 AM

Edit: too slow, too slow...

That would be a horrible work-around. Objects you put into C++SL containers must be copyable and assignable (or movable in C++11). That is usually not a problem because if you put something like std::string into your class it will be handled correctly (because the automatically generated copy and assignment operators of MyClass call the appropriate copy or assignment of std::string) since std::string is (like practically everything in the C++SL) copyable and assignable (and movable in C++1).
Where it will be a problem is with allocated memory. If you allocate memory in the object and store the pointer you will usually free the memory in the destructor. But the default copy constructor/assignment operator will just copy the pointer, no whatever it points to. So continuing the example from above, as soon as U is destroyed the copy points to invalid memory (because it was freed in U's destructor). There are several ways to deal with that:
1) make a deep copy of the allocated memory (that is, the copy constructor and assignment operator must allocate the data themselves and copy the copy over from the source).
2) implement reference counting of some kind so that the data is not deleted until the last instance holding it gets destroyed.
3) use something like boost::shared_ptr or std::shared_ptr (in C++11).

It is worth mentioning the Rule of Three here. If any of your classes needs any of a handcrafted copy-constructor, assignment operator or destructor you in general need to handcraft all three.

Hodgman

52,717

July 31, 2012 09:30 AM

So, should I ignore the destructors and call a function within the class to handle the cleanup instead?

No. When you create a temporary and copy it into a vector, the destructor is still only called once on each instance of the class. You're seeing it be called twice because you've made two instances -- one temporary and one inside the vector.
This is the correct behaviour when using a temporary.

. 22 Racing Series .