Reset iterator inside loop with erase method

Question

Here's a little piece of code, which is making us a little mad...

for (vector<QSharedPointer<Mine>>::iterator itMine = _mines.begin(); 
       itMine != _mines.end();) {
    auto point2 = shipFire[l].getBulletLine().p2();
    if (_mines[pos]->checkCollision(point2)) { // _mines is a Vector<QSharedPointer<Mine>>
        Explosion explosionMine(_mines[pos]->point());
        _explosions.push_back(explosionMine);
        itMine = _mines.erase(itMine);
        bulletErased = true;
        scoreFrame += 100;
    }
    else {
        ++itMine;
        pos++;
    }
}

Here is the problem, the itMine is erasing two of our vector<..<Mine>> and it's making the program to shutdown unexpectedly

We thought about it, and came up with this : All of our iterators are being invalidated after erasing the one of the Mine, right ?

But we are a little confused about how to change our actual code to fit to the new one ?

The main question is : how do we reset this itr ?

ps : if you have any questions, or if you need a little more of code to understand the logic behind, feel free to ask more question !

Best Regards, and thank you in advance.

`_mines[pos]` and use of iterators kind of clashes. You might want to rethink that. Why not use the iterator all the way through? — user4581301, Feb 03 '16 at 19:29
[This](https://ideone.com/HKcjSM) is logically similar to what you have and works just fine (i.e., you can increment the iterator or use the one returned by the `erase` function to safely loop all the elements). — James Adkison, Feb 03 '16 at 19:32
And [look in into whether the Remove-Erase idiom is right for you](http://stackoverflow.com/questions/347441/erasing-elements-from-a-vector) — user4581301, Feb 03 '16 at 19:35

Thomas · Answer 1 · 2016-02-03T22:05:54.557

0

A not uncommon solution to this problem is to swap the current position with the back position. The last item can then be removed without invalidating any iterators. See live example.

#include <iostream>
#include <vector>
#include <memory>

int main()
{
    struct Mine
    {
        Mine( int id = 0 ): id( id ) { }
        bool check_collision_point( int point ) const { return id % point == 0; }
        int id; 
    };

    auto mines = std::vector<std::shared_ptr<Mine>>{ 7 };

    for( auto i = 0u; i < mines.size(); ++i )
    {
        mines[i] = std::make_shared<Mine>( i );
    }

    // save point outside loop
    auto point = 2;

    for( auto it = mines.begin(); it != mines.end(); )
    {
        // don't use mines[pos] in loop
        if( (*it)->check_collision_point( point ) ){
            std::iter_swap(it, mines.rbegin() );
            mines.resize( mines.size() -1 );
        }
        else
            ++it;
    }

    for( auto it = mines.begin(); it != mines.end(); ++it)
        std::cout << (*it)->id << " "; 
}

edited Feb 03 '16 at 22:05

answered Feb 03 '16 at 19:38

Thomas

4,980
2
15
30

1

[This](http://coliru.stacked-crooked.com/a/fcd311d0b38b12e9) works just fine without needing to use swap. I'll also mention that the erase remove idiom mentioned by @user4581301 in the main comments may be useful to the OP (i.e., instead of manually swapping and erasing). – James Adkison Feb 03 '16 at 19:50
@James Adkison I agree that your solution is more compact and elegant, but it will be more inefficient when few items are removed from a large vector. – Thomas Feb 03 '16 at 20:31
Yes, that's a reason to use the erase remove idiom. I was only pointing out that your answer is not required to solve the OPs problem. – James Adkison Feb 03 '16 at 20:33
In our case, our _mines a QSharedPointer, not a Int, so we are getting an invalid operand to binary conversion error. since the *itMine can't be casted to int, this solution don't work with us, can you specify more in our case if possible ? – Rolljee Feb 03 '16 at 21:05
Further edit to mimic comparison against against fixed member of mines. – Thomas Feb 03 '16 at 21:37

score 0 · Accepted Answer · answered Feb 06 '16 at 11:07

Update

   for (auto itMine = _mines.begin(); itMine != _mines.end();){
            auto point2 = shipFire[l].getBulletLine().p2(); // thomas : -> here we need to do it in the loop, to get every bullet position while looping.

            if (_mines[pos]->checkCollision(point2)){
                Explosion explosionMine(_mines[pos]->point());
                _explosions.push_back(explosionMine);
                if (_mines[pos]->checkCollision(point2)){
                    mines_to_be_deleted.push_back(*itMine);
                    shipFire.erase(it);
                    bulletErased = true;
                    scoreFrame += 100;
                    qDebug() << "Collision bullet mine";

                }
            }
            //else
            ++itMine;
            pos++;
        }


        for (auto itrDelete = mines_to_be_deleted.begin(); itrDelete != mines_to_be_deleted.end();)
        {
                _mines.pop_back(); // Here instead of that, we need to erase the current itr (here is our mistake) 
                //mines_to_be_deleted.erase(itrDelete);
                //_mines.resize(_mines.size());
                qDebug() << "mine deleted";
                ++itrDelete;
        }
        mines_to_be_deleted.clear();

At this point we came up with this. We tried everything you told us, but none of them worked, we thinked about swapping the current itr to the end of the vector. Thanks for your help.

For now, the program is running perfectly, but when we hit a Mine with a bullet, a ramdom one disapear.

For those who want to know, we have found the solution.

while(!mines_to_be_deleted.empty()) { // _mine_to_be_deleted is a QList<Sharedpointer<Mine>>
        std::remove(begin(_mines),end(_mines),mines_to_be_deleted.front());
        _mines.pop_back();
        mines_to_be_deleted.pop_front();
    }

using std::remove allow us to erase the Mine without creating a malloc()

Reset iterator inside loop with erase method

2 Answers2