Sign in to follow this  
lonewolff

Safely deleting a class instance

Recommended Posts

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?

[code]for(it=VectorAsset.begin();it<VectorAsset.end();it++)
{
if(szNameOrType==it->szName||szNameOrType==it->szType)
{
//delete it; // <<< wont compile
VectorAsset.erase(it);
}
}[/code]

Any help would be awesome :)

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Thanks for the reply.

I allocated the instances like this;

[code]Asset *cAsset=new Asset(szName,szTexture);
VectorAsset.push_back(*cAsset);[/code]

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

Compiler give this error

[b]error C2440: 'delete' : cannot convert from 'Asset' to 'void *'[/b]

This is what I have now;

[code]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++;
}
}[/code]

[edit]
delete &it; worked, though. Edited by lonewolff

Share this post


Link to post
Share on other sites
really? It worked?
That seems to be a coincidence.

[code]
Asset *cAsset=new Asset(szName,szTexture);
VectorAsset.push_back(*cAsset);
[/code]

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

Share this post


Link to post
Share on other sites
[quote name='lonewolff' timestamp='1343716472' post='4964732']
[edit]
delete &it; worked, though.
[/quote]

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

Share this post


Link to post
Share on other sites
[quote name='lonewolff' timestamp='1343716472' post='4964732']
delete &it; worked, though.
[/quote]no no no no! You're trying to [font=courier new,courier,monospace]delete[/font] memory that you didn't allocate with [font=courier new,courier,monospace]new[/font]! That memory belongs to the vector/iterator, not to you.

[quote][code]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)[/code][/quote]With a vector of values, this should just be:[code]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.[/code]
OR
you should use a vector of pointers, in which case new/delete would work as you expect them to:[code]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[/code] Edited by Hodgman

Share this post


Link to post
Share on other sites
Thanks for the advice guys!

I have change the code to the following

[b]std::vector<class Asset>(VectorAsset);[/b]

So, when I call [b]VectorAsset.erase(it); [/b]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).

Share this post


Link to post
Share on other sites
[quote name='lonewolff' timestamp='1343718777' post='4964747']
Thanks for the advice guys!

I have change the code to the following

[b]std::vector<class Asset>(VectorAsset);[/b]

So, when I call [b]VectorAsset.erase(it); [/b]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).
[/quote]

Yes, the destructor will get called.

Share this post


Link to post
Share on other sites
A wierd one. Testing my destructor, it is actually getting called when the class is created. Any ideas why that would be?

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

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
Whenever you insert something into the vector with (for example) [code]myVector[i] = MyClass(...)[/code] or [code]myVector[i].push_back(MyClass(...))[/code] 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.

Share this post


Link to post
Share on other sites
Your loop is incorrect. It allows invalidated iterators to be accessed. It should be:
[code]
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;
}
}
}
[/code]
Note the assignment to "it" of the return value of "erase()".

[quote]
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.

[quote]
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.

[quote]
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.

[quote]
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).

[quote]
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 [url="http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)"]rule of three[/url]. Edited by rip-off

Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
[quote name='lonewolff' timestamp='1343724935' post='4964771']
So, should I ignore the destructors and call a function within the class to handle the cleanup instead?
[/quote]No. When you create a temporary and copy it into a vector, the destructor is still only called once [i]on each instance of the class[/i]. 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.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this