Jump to content

  • Log In with Google      Sign In   
  • Create Account


Safely deleting a class instance


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
15 replies to this topic

#1 DarkRonin   Members   -  Reputation: 604

Like
0Likes
Like

Posted 31 July 2012 - 12:20 AM

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 :)

Sponsor:

#2 SimonForsman   Crossbones+   -  Reputation: 5753

Like
1Likes
Like

Posted 31 July 2012 - 12:26 AM

How did you allocate those class instances ?

If you used new to allocate them you should be able to delete them using

delete *it; // it is a interator pointing to your pointer so you have to dereference it to get your pointer.
then remove the iterator using it = VectorAsset.erase(it); //you have to reassign the iterator since erase will invalidate it.

If you didn't allocate your instances with new you can just call erase and things will get cleaned up, (You still have to reassign the iterator though.
I don't suffer from insanity, I'm enjoying every minute of it.
The voices in my head may not be real, but they have some good ideas!

#3 DarkRonin   Members   -  Reputation: 604

Like
0Likes
Like

Posted 31 July 2012 - 12:34 AM

Thanks for the reply.

I allocated the instances like this;

Asset *cAsset=new Asset(szName,szTexture);
VectorAsset.push_back(*cAsset);

But, Still can't call delete (where commented above);

Compiler give this error

error C2440: 'delete' : cannot convert from 'Asset' to 'void *'

This is what I have now;

void SpriteUnload(string szNameOrType)
{
  // Erase any valid items from vector array
  std::vector<Asset>::iterator it;
  for(it=VectorAsset.begin();it<VectorAsset.end();)
  {
   if(szNameOrType==it->szName||szNameOrType==it->szType||szNameOrType=="ALL")
   {
	delete *it;
	//VectorAsset.erase(it);
	VectorAsset.erase(it);
   }
   else
	it++;
  }
}

[edit]
delete &it; worked, though.

Edited by lonewolff, 31 July 2012 - 12:36 AM.


#4 Madhed   Crossbones+   -  Reputation: 2487

Like
2Likes
Like

Posted 31 July 2012 - 12:41 AM

really? It worked?
That seems to be a coincidence.

Asset *cAsset=new Asset(szName,szTexture);
VectorAsset.push_back(*cAsset);

These 2 lines actually push a copy of the asset in the vector. The original Asset pointer is lost in the process.
If you want to store asset pointers use a vector<Asset*> not vector<Asset> and directly push the pointer (without dereferencing).

Since you're posting in "for beginners" I would recommend you read up on smart pointers, especially auto_ptr which is useful in this case.

Edited by Madhed, 31 July 2012 - 12:43 AM.


#5 SimonForsman   Crossbones+   -  Reputation: 5753

Like
1Likes
Like

Posted 31 July 2012 - 12:41 AM

[edit]
delete &it; worked, though.


Thats because you were dereferencing your pointer before pushing it to the vector, which would AFAIK create a copy of it (giving you a memory leak).

If you are going to store raw pointers you should use std::vector<Asset *> , if you use std::vector<Asset> you shouldn't allocate them using new.

Edit: Beaten by madhet,

Anyway, if you use std::vector<Asset *> instead it will work with delete *it; and you won't get a memory leak.

You could also look into using smart pointers to avoid the hassle.

Edited by SimonForsman, 31 July 2012 - 12:43 AM.

I don't suffer from insanity, I'm enjoying every minute of it.
The voices in my head may not be real, but they have some good ideas!

#6 Hodgman   Moderators   -  Reputation: 27585

Like
4Likes
Like

Posted 31 July 2012 - 12:42 AM

delete &it; worked, though.

no no no no! You're trying to delete memory that you didn't allocate with new! That memory belongs to the vector/iterator, not to you.

Asset *cAsset=new Asset(szName,szTexture);
VectorAsset.push_back(*cAsset);//makes a new Asset inside the vector, copies your new asset's value the vector's new asset's value
//your original new asset is leaked (cAsset is not put into the vector)

With a vector of values, this should just be:
Asset cAsset(szName,szTexture);//make a new temporary asset on the stack (new not required). This temporary one will be destructed when it goes out of scope.
VectorAsset.push_back(cAsset);//create a new asset in the vector, copy yours into that new one
//N.B. new/delete aren't required. Destructor will be called upon erasing items from the vector.
OR
you should use a vector of pointers, in which case new/delete would work as you expect them to:
Asset *cAsset=new Asset(szName,szTexture);
VectorAsset.push_back(cAsset);//put the pointer into the vector
...
it=VectorAsset.begin()
...
delete *it;//retrieves your original pointer, deletes the allocation that it points to

Edited by Hodgman, 31 July 2012 - 12:56 AM.


#7 DarkRonin   Members   -  Reputation: 604

Like
0Likes
Like

Posted 31 July 2012 - 01:12 AM

Thanks for the advice guys!

I have change the code to the following

std::vector<class Asset>(VectorAsset);

So, when I call VectorAsset.erase(it); does the destructor get called still? As I have alot of DirectX code that needs to be cleaned up via the destructor (in each instance).

#8 SimonForsman   Crossbones+   -  Reputation: 5753

Like
0Likes
Like

Posted 31 July 2012 - 01:58 AM

Thanks for the advice guys!

I have change the code to the following

std::vector<class Asset>(VectorAsset);

So, when I call VectorAsset.erase(it); does the destructor get called still? As I have alot of DirectX code that needs to be cleaned up via the destructor (in each instance).


Yes, the destructor will get called.
I don't suffer from insanity, I'm enjoying every minute of it.
The voices in my head may not be real, but they have some good ideas!

#9 DarkRonin   Members   -  Reputation: 604

Like
0Likes
Like

Posted 31 July 2012 - 02:08 AM

A wierd one. Testing my destructor, it is actually getting called when the class is created. Any ideas why that would be?

~Asset()
{
  std::cout<<"Destructing: "<<szName<<"\r\n";
}


#10 Ohforf sake   Members   -  Reputation: 1442

Like
-1Likes
Like

Posted 31 July 2012 - 02:43 AM

When you resize a vector, some implementations create a dummy instance, then use that dummy instance in the copy constructor of every new instance in the vector and then free the dummy instance. I guess it is that behavior, that you are seeing.

#11 BitMaster   Crossbones+   -  Reputation: 3659

Like
0Likes
Like

Posted 31 July 2012 - 02:49 AM

Whenever you insert something into the vector with (for example)
myVector[i] = MyClass(...)
or
myVector[i].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.

#12 DarkRonin   Members   -  Reputation: 604

Like
0Likes
Like

Posted 31 July 2012 - 02:55 AM

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

#13 rip-off   Moderators   -  Reputation: 7642

Like
4Likes
Like

Posted 31 July 2012 - 03: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);

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?

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).

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.

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?

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.

Edited by rip-off, 31 July 2012 - 03:03 AM.


#14 BitMaster   Crossbones+   -  Reputation: 3659

Like
0Likes
Like

Posted 31 July 2012 - 03: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.

Edited by BitMaster, 31 July 2012 - 03:07 AM.


#15 Hodgman   Moderators   -  Reputation: 27585

Like
1Likes
Like

Posted 31 July 2012 - 03: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.

#16 Tallkotten   Members   -  Reputation: 288

Like
0Likes
Like

Posted 01 August 2012 - 03:48 AM

too slow... rip-off said it right :)




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS