Sign in to follow this  
Funkyjive

STL Iterator problems

Recommended Posts

I seem to be having a spot of trouble dealing with erasing elements from an STL vector. The vector erase function requires a vector instead of just an index to delete. In my class I have a member variable as follows...
vector<Character*> units;
The vector gets populated as follows...
for(int i=0; i<NUM_UNITS; i++)
{
    Character* tmpchar = new Character;
    tmpchar->Load("PlayerModel.bmf", "ZombieTexture.tga", ZOMBIE_UNIT);
    units.push_back(tmpchar);
}
Here is where my error is coming from...
//Delete any dead units
for (vector<Character*>::iterator it = units.begin(); it!=units.end(); it++)
{
    if(it->GetHealth() <= 0)
    {
        //Unload the unit
        delete ⁢
        units.erase(it);
    }
}
The compiler shouts at me in the if statement. I have no idea how to interpret this error message. error: request for member `GetHealth' in `*(&it)->__gnu_cxx::__normal_iterator<_Iterator, _Container>::operator-> [with _Iterator = Character**, _Container = std::vector<Character*, std::allocator<Character*> >]()', which is of non-class type `Character*'| My thinking in the if statement is that the iterator is of type Character* in which case why can I not call the Character member function GetHealth? The GetHealth function is indeed there but I think the message isin't complaining about the function so much as it doesn't think its a Character pointer. Any Help? Thanks in advance!

Share this post


Link to post
Share on other sites
You use the * operator to access the element pointed too by the iterator.

Try changing

delete ⁢

to

delete *it;

and

if(it->GetHealth() <= 0)

to

if((*it)->GetHealth() <= 0)

[edit]

A recommendation, you might think about replacing your storage of Character pointers with smart_ptr's instead. It would eliminate the need to have the delete line, all you would have to do is erase the element from the vector. Once it has been removed, and there are no other references to it, it would automatically be deleted.

Share this post


Link to post
Share on other sites
This is NOT how you erase elements from a vector while you're iterating over it:


//Delete any dead units
for (vector<Character*>::iterator it = units.begin(); it!=units.end(); it++)
{
if(it->GetHealth() <= 0)
{
//Unload the unit
delete ⁢
units.erase(it);
}
}


In particular, once you call units.erase(it), that iterator is invalidated and you're not to do anything with it. However, at the top of your loop you then proceed to increment it. This is garbage.

.erase() returns the next valid iterator so you can correctly continue iterating over the sequence. Remember that erasing something from the middle of the vector causes all elements after that to be copied.

The correct way to erase while iterating is to use the return value from .erase() and not increment the iterator in the loop update:


for (vector<T>::iterator it = vec.begin(); it != vec.end(); )
{
if( shouldErase )
{
it = vec.erase(it);
}
else
{
++it;
}
}


If you're often removing from the middle, vector might be the wrong container choice

Share this post


Link to post
Share on other sites
Quote:
Original post by Rattrap
//Omitted

A recommendation, you might think about replacing your storage of Character pointers with smart_ptr's instead. It would eliminate the need to have the delete line, all you would have to do is erase the element from the vector. Once it has been removed, and there are no other references to it, it would automatically be deleted.


Thanks for the quick reply. I will have to look into smart pointers. I have heard of them but never knew what purpose they had.


Quote:
Original post by RDragon1
This is NOT how you erase elements from a vector while you're iterating over it:

//Omitted

If you're often removing from the middle, vector might be the wrong container choice


Thanks for the correction. I was aware from one of my books that deleting mid vector was not very efficient. Right now however I am unsure of the gameplay and it might not be an issue (Only 1 or 2 zombies being killed every few seconds). However, if it should present an issue such as a case where the player is spraying shotgun into a crowd and multiple units are killed every game update, what would be a suggestion for a better structure to hold the references? I could do a fixed length array and zero out the memory for a slot if it is deleted, but are there any better containers for this type of situation?

Thanks!

Share this post


Link to post
Share on other sites
I'd go so far as to suggest:

class CharRemoveTest
{
public:
bool operator ()(Character *c) { return (c->GetHealth() < 0.0f); }
}

std::erase( std::remove_if( units.begin(), units.end(), CharRemoveTest ), units.end );


it is faster, so long as the order of units in the array doesn't matter.
Since it swaps all the dead ones to the end of the array, so erase can just lop off the tail end.

Share this post


Link to post
Share on other sites
KulSeran's suggestion works IF the container either

1) contains smart pointers
2) doesn't own the Character objects (and isn't responsible for deleting them)


remove() does NOT "swap all the dead ones to the end of the array", it moves non-removed elements up towards the beginning of the array, replacing removed ones. What's at the end of the array is undefined, and most likely is exactly what was there before remove().

So, if you use erase-remove on the container of raw pointers, you risk leaking memory, unless this container isn't responsible for deleting them

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