What is proper way to delete objects that resides in a list that you find while looping that list?

Question

I have a list of Star structs. These structs are in a std::list

I am double looping this list and compairing there locations to detect a collision. When A collision is found I will delete Star with the lowest mass. But how can I delete the Star when I am in the double Loop, and keep the loop going to check for more collisions?

It's worth mentioning that the second loop is a reverse loop.

Here is some code

void UniverseManager::CheckCollisions()
{
    std::list<Star>::iterator iStar1;
    std::list<Star>::reverse_iterator iStar2;
    bool totalbreak = false;

    for (iStar1 = mStars.begin(); iStar1 != mStars.end(); iStar1++)
    {
        for (iStar2 = mStars.rbegin(); iStar2 != mStars.rend(); iStar2++)
        {
            if (*iStar1 == *iStar2)
                break;
            Star &star1 = *iStar1;
            Star &star2 = *iStar2;

            if (CalculateDistance(star1.mLocation, star2.mLocation) < 10)
            {
                // collision
                // get heaviest star
                if (star1.mMass > star2.mMass)
                {
                    star1.mMass += star2.mMass;
                    // I need to delete the star2 and keep looping;
                }
                else
                {
                    star2.mMass += star1.mMass;
                    // I need to delete the star1 and keep looping;
                }

            }
        }

        }
}

Your algorithm as written will think that every star has collided with itself. Consider replacing `iStar2 != mStars.rend()` with `iStar2 != iStar1` — Drew Dormann, Jan 02 '12 at 20:45
Thats what the first line is if ( *iStar1 == *iStar2) break; But your way is cleaner. So I'll change it. Doesn't answer my question though. — EddieV223, Jan 02 '12 at 20:48
Ah, I see that now. Consider the change nonetheless because otherwise you'll be comparing each pair of stars twice. — Drew Dormann, Jan 02 '12 at 20:49
Also note that if you decide to delete Star1, you **do not** want to keep looping the inner loop. You'll be comparing more Stars against the one you just deleted. — Drew Dormann, Jan 02 '12 at 20:54
Well since the double loop with reverse iterators are pretty much impossible to do this I have just added a bool to the stars and set it to true if I want to delete one, then later I'll loop the list with a single iterator, check for the bool and then do the iter = mStars.erase(iter); Seems like the most logical way to do this. — EddieV223, Jan 03 '12 at 04:04

Ian · Accepted Answer · 2012-01-02T20:54:28.970

2

You need to utilize the return value of the erase method like so.

iStar1 = mStars.erase(iStar1);
erase = true;
if (iStar1 == mStars.end())
   break; //or handle the end condition

//continue to bottom of loop

if (!erase)
   iStar1++; //you will need to move the incrementation of the iterator out of the loop declaration, because you need to make it not increment when an element is erased.

if you don't increment the iterator if an item is erased and check if you deleted the last element then you should be fine.

edited Jan 02 '12 at 20:54

answered Jan 02 '12 at 20:48

Ian

4,169
3
37
62

So if I understand correctly, calling erase will, delete the object it currently points to and then returns the next element in the list? Which you should make = the iterator? Does this work the same for a reverse::iterator? In that If I have reverse::iterator rit; i would rit = mStars.erase(rit); and NOT ++ it? – EddieV223 Jan 02 '12 at 20:53
It doesn't actually delete the pointer to deallocate the memory pointed to, it just removes the pointer from the list. You can either delete the memory manually or create a smart container that does this for you. And it should work the same for the reverse iterator as you've indicated. – Ian Jan 02 '12 at 20:56

score 0 · Answer 2 · answered Jan 02 '12 at 20:51

Since modifying the list invalidates the iterators (so that you cannot increment them), you have to keep safe the iterators before the list is changed.

In the most of the implementation std::list is a dual-linked list, hence a iteration like

for(auto i=list.begin(), ii; i!=list.end(); i=ii)
{
    ii = i; ++ii; //ii now is next-of-i

    // do stuff with i

    // call list.erasee(i).
    // i is now invalid, but ii is already the "next of i"
}

The safest way, is to create a list containing all the "collided", then iterate on the "collided" calling list.remove(*iterator_on_collided) (but inefficient, since has O² complexity)

score 0 · Answer 3 · answered Jan 02 '12 at 20:53

You want to use the result of erase() to get the next iterator and advance the loop differently:

If you erase using the outer iterator you clearly can abondon checking this Star against others and break out of the inner loop. Only if the inner loop was complete you'd want to advance the outer iterator because otherwise it would be advanced by the erase().
If you erase using the inner loop you already advanced the iteration, otherwise, i.e. if no star was erased, you need to advance.

Sample code would look somethimg like this:

for (auto oit(s.begin()), end(s.end()); oit != end; )
{
    auto iit(s.begin());
    while (iit != end)
    {
        if (need_to_delete_outer)
        {
            oit = s.erase(oit);
            break;
        }
        else if (need_to_delete_inner)
        {
            iit = s.erase(iit);
        }
        else
        {
            ++iit;
        }
    }
    if (iit == end)
    {
        ++oit;
    }
}

What is proper way to delete objects that resides in a list that you find while looping that list?

3 Answers3