Jump to content
  • Advertisement
Sign in to follow this  
DarkCybo1

Program is seg faulting when I delete from an std::list (C++)

This topic is 4698 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

The update function; Happens every loop.
[source lang = "c++"]
void cGameState::doUpdate()
{
   
   std::list<cSceneEntity * > * listPtr = m_Scene.GetEntityList();
   std::list<cSceneEntity *>::iterator iter = listPtr->begin();
   
   for(; iter != listPtr->end(); iter++)
   {
      
      if((*iter)->GetObjectType() == PARTICLE)
         updateParticle((cParticle *)(*iter));

      if(!(*iter)) //This for some reason never is true, even if I delete it in the other function.
         iter = listPtr->begin();

   }
   
}

Particle update function.
[source lang ="c++"]
void cGameState::updateParticle(cParticle * part)
{
   
   if(m_bParticlesDie)
   {
      
      part->updateLife();

      //If part->m_a is 0.
      if(!part->m_a)
      {
         
         m_Scene.removeEntity(part);
         
         m_particleCount--;
         part = NULL;
         
      }
      
   }
   
}


Entity removal.
[source lang = "c++"]
inline void cScene::removeEntity(cSceneEntity * entity)
{
   
   m_pSceneEntities.remove(entity);
   
   delete entity;
   
}


It crashes in the doUpdate function, sometime after I call removeEntity.

Share this post


Link to post
Share on other sites
Advertisement
The std::list<>'s remove() call will invalidate any and all iterators, pointers, and references pointing to the element being removed (in case of a vector, for instance, it'd also invalidate a lot more). This fraks up your loop.

So you shouldn't use remove() like that; even if it did work, it has to do a potentially expensive linear search to find the element you give it (you will probably have thousands or tens of thousands of elements in your list -- SLOOWWW). Instead, use the erase() function that takes an iterator pointing to the desired element.

The pointer is not null (nb: you should use 0, not NULL, in C++) because in updateParticle(), you are modifying a copy of the pointer, not the "original" one.

This is the correct way of removing things from a list while iterating through it:

while(iter != list->end())
{
if(/* needs to be removed */)
{
delete *iter;
//
// Important! erase returns a valid iterator pointing to the next element
// You cannot use the old iterator anymore because it is invalidated
//
iter = list.erase(iter);
}
else
++iter;
}


Your code could use some general refactoring. First of all, updating a particle should be the job of the particle itself, not of the "game state". That is, "update()" should be a member function and fields like m_a (whatever that is - nondescriptive variable names are Bad(tm)) should be private.

Secondly, you should use polymorphism instead of reinventing run-time type identification through ugly queries and if-else-constructs.

Thirdly, if your objects always reside in a list somewhere, why pass them around by pointers - just pass an iterator!

Fourth, there are some algorithms in the Standard Library that can be used in loops like this; take a look at std::remove_if, for example.

Share this post


Link to post
Share on other sites
Is m_pSceneEntities also a list?


What do you do this for?
if(!(*iter)) //This for some reason never is true, even if I delete it in the other function.
That will never happen unless you put a 0 pointer into the list. You check iterators against container.end() to see if it's the last one.

When you call remove, it removes it from the list and stiches the list so all the iterators remain valid (except the one removed).


Oh, you can't do that. Not like that anyway. You're remove elements from the list in a sub-function that's called from the loop that iterators that list. You are most likely invalidating the iterator that is in use, then the loop tries to do the ++ on it and it blows up.

The right way to remove an element from the list while iteratoring it is something like this:


for(it=l.being(); it!=l.end(); ++it)
{
if(it->something())
{
it = l.erase(it);
--it; //erase returns the next element, and we'll increment and skip it with the loop
continue;
}
}

Share this post


Link to post
Share on other sites
Quote:
Original post by Shannon Barber

for(it=l.being(); it!=l.end(); ++it)
{
if(it->something())
{
it = l.erase(it);
--it; //erase returns the next element, and we'll increment and skip it with the loop
continue;
}
}


What is you erase the first item in the list? You will end up with it being set to the first item and think call --it to it. Other cases can exist where this could cause problems.

Instead of using a for loop, you should use a while loop


it = l.begin();
while(it != l.end())
{
if(it->something())
{
it = l.erase(it);
}
else
it++;
}


This way if an element is remove, it won't advance until the next iteration, because it hasn't evaluated whether the one it set it to is "something()".

-edit- Just realized Sharlin wrote pretty much that exact code [sad]

Also consider boost::ptr_list

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!