vector::erase causing strange behavior

Question

I am trying to create a vertically scrolling shooter where when you press space a bullet is created and then when the bullet goes off screen the bullet is destroyed. I keep track of the bullets through vector delcared as vector<BULLET> bullets; When I try to destroy any bullets that are outside of the screen, I get a ton of errors such as: c:\mingw\bin\..\lib\gcc\mingw32\4.7.0\include\c++\bits\stl_algobase.h|384| required from '_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = true; _II = BULLET*; _OI = BULLET*]'|

My code looks like this:

for( auto it = bullets.begin(); it != bullets.end(); ){
    if( it->is_dead()){
        it = bullets.erase(it);
    }else{
        it++;
    }
}

The part that's frustrating me is that I have similar loop that deletes any game objects that need to be deleted in a vector that holds pointers with:

for( auto it = activeInstances.begin(); 
it != activeInstances.end(); ){
    if( (*it)->is_dead()){
        it = activeInstances.erase(it);
    }else{
        it++;
    }
}

and this one works just fine.

Edit: I'm not sure if it makes a difference or not but just for reference I'm adding the section that occurs later in the same function that adds bullets to the vector:

if( key[SPACE] && reload == 0){
    reload = reloadTime;
    BULLET newBullet;
    newBullet.init( x, y);
    bullets.push_back( newBullet);
}

Maybe you should replace if( it->is_dead()){ with exactly the same code from bellow loop if( (*it)->is_dead()){ ... You must access object in iterator and that you could do with *it — Igor, Jul 15 '13 at 09:22
`it->` is valid to access the object of the iterator but if the objects in the vector itself is a point type, you'll have to go via `(*it)->` which in fact dereferences twice: the iterator and whatever type the iterator holds. Is BULLET a pointer type? — Pixelchemist, Jul 15 '13 at 09:24
I did try chaning `it->is_dead()` to `(*it)->is_dead()` just to make sure that wasn't the problem and that throws an error because bullets is a vector of BULLET and activeInstances is a vector of *OBJECT — Justin McFall, Jul 15 '13 at 18:03
The problem WAS that I didn't have a move constructor for BULLET. I didn't even know what that was, but after I read about it, it makes sense. Thank you for all your help! — Justin McFall, Jul 15 '13 at 18:46

score 1 · Answer 1 · answered Jul 15 '13 at 09:20

1

your code fragements differ:

if( it->is_dead()){

vs.

if( (*it)->is_dead()){

answered Jul 15 '13 at 09:20

collapsar

17,010
4
35
61

So do the the vectors that are being used by these snippets. This is by itself not the problem. – jrok Jul 15 '13 at 10:37
1

you're right, the definition of `BULLET` is crucial, but the code diff hints to a probable cause of the error. – collapsar Jul 15 '13 at 10:47
If you could elaborate on that last statement collapsar I would really appreciate it. One is referencing a vector of objects and the other is referencing a vector of pointers to objects so that being the only difference in the code shouldn't be causing any problems, right? – Justin McFall Jul 15 '13 at 18:25
@JustinMcFall: you didn't provide the definition of BULLET but stressed the analogy to the code segment with `activeInstances`. Glad you could sort out your problem following doctorlove's hint. – collapsar Jul 15 '13 at 19:15

score 1 · Answer 2 · edited May 23 '17 at 12:29

I would suggest using the erase-remove idiom to remove items from std::vector (you can consider this StackOverflow Q&A for a detailed discussion, also applied to other containers):

// Erase elements matching "erasing_condition" from vector "v"
v.erase( std::remove_if(v.begin(), v.end(), erasing_condition), v.end() );

In your particular case, you may want to use some code like this:

//
// Erase elements matching "BULLET.is_dead()" from vector "bullets".
// ("bullets" is a "vector<BULLET>")
//
bullets.erase
(     
    std::remove_if
    ( 
        bullets.begin(), 
        bullets.end(), 

        // Erasing condition
        [](const BULLET& bullet)
        { 
            return bullet.is_dead(); 
        }   
    ), 
    bullets.end() 
);

BTW: As a style guide, I'd prefer Bullet instead of BULLET for a C++ class name (ALL_UPPERCASE_STYLE is usually reserved for preprocessor macros in C++...).

I do appreciate the style guide very much, I agree that would be much better. I also tried your suggestion and still errors are thrown. The first one is new and is: **stl_algo.h 1158** `error: no match for 'operator=' in '__result.__gnu_cxx::__normal_iterator<_Iterator, _Container>::operator* >() = std::move((* & __first.__gnu_cxx::__normal_iterator<_Iterator, _Container>::operator* >()))' candidate is: BULLET& BULLET::operator=(BULLET&)` After that I get the same error as in OP flagged at the final line of bullets.erase — Justin McFall, Jul 15 '13 at 18:18

score 0 · Answer 3 · answered Jul 15 '13 at 18:48

The problem, as someone mentioned in one of the comments, was that I wasn't using a move constructor in my BULLET class (I'm a total noob and didn't even know what that was until it was mentioned). I really appreciate everyone's help though, I actually learned a lot!

vector::erase causing strange behavior

3 Answers3