🔒 Locked

Iterators and Delete?

Started by endasil Aug 23, 2005 at 5:17 PM 2 replies 900+ views

Original Post

Aug 23, 2005 at 5:17 PM


//--------------------------------------------------------------------
// Animate entities
//--------------------------------------------------------------------

for(int iLayerNr = 0; iLayerNr < MAX_ENTITY_LAYERS;iLayerNr++)
{

    map<uint,CEntity*> ::iterator next = mEntities[iLayerNr].begin();

    while(next != mEntities[iLayerNr].end())
    {
	int retval = 0;
	
        if(next->second->hasAttribute(CEntity::ATTRIBUTE_ANIMATED))
	{
		retval = next->second->update(newtime, lasttime);
	}
	if(retval == -1)
	{

        //--------------------------------------------------------------
	//Returned -1, entity wants us to delete it...
	//--------------------------------------------------------------------
	        delete next->second;
		map<uint,CEntity*> ::iterator curr = next++;
	        mEntities[iLayerNr].erase(curr);
	}
	else
	{	
	        next++;
	}
    }
}

the question i have is about this part. It seems like he is erasing the next entity and not the one that requested to be erased?



        delete next->second;
	map<uint,CEntity*> ::iterator curr = next++;
        mEntities[iLayerNr].erase(curr);

should it not be like this or is there something i missed?



        delete next->second;
        mEntities[iLayerNr].erase(curr);
	map<uint,CEntity*> ::iterator curr = next++;

// edit - added a title [Edited by - LessBread on August 23, 2005 7:01:22 PM]

jflanglois

Aug 23, 2005 at 5:26 PM

No, you are reading the code literally. I suppose "next" is a misnomer. It would have been better named as "current".

You may want to read up on STL iterators, particularly iterators for maps, which use std::pairs to deal with the pointed to element (and thusly the use of next->second in the code which can be even more misleading).

Where does this code come from?

[edit] I did not read everything. Also note that in the line map ::iterator curr = next++;, the author is using the postfix increment operator, so curr is getting the value of next and then next is being incremented. It is a little strange in the readability department, but it does what it is supposed to do. I would have done something like this instead:

delete next->second;next = mEntities[iLayerNr].erase( next );

jfl.

endasil

Aug 23, 2005 at 6:04 PM

Ah, next++ increases after the value has been given, that changes things. I see the code very differently now that i know that. Thanks alot!

The code comes from my uni teacher in 2d game engine development, who is currently on vacation so i could not ask him about his code.

jflanglois

Aug 23, 2005 at 6:35 PM

For clarification:

If you did not want the value to be incremented after assignment, you would use the prefix increment operator, that is ++next instead of next++.

Also, map::erase() -- or at least the form that is in use -- will return an iterator to the next element or the end of the container (map::end()), if there are no more elements. I see no reason for creating a temporary iterator (curr) except to show explicitly that the next iterator has changed. In this case, however, it seems to have made things more confused.

jfl.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.