CODEWITHSUNDEEP
c++vectoriterator
celphy
celphy

Reputation: 127

C++ Vector.erase() last element corrupts iterator

I currently have a problem with vector.erase().

vector<gameObject> gameObjects;

for (auto it = gameObjects.end() - 1; it != gameObjects.begin();)
    {
        if ((it)->getDestroyed()) {
            it = gameObjects.erase(it);
        }
        else {
            --it;
        }
    }

So gameObject is the base class for everything inside the game and it has a bool flag that basically tells us if the object was destroyed. If the flag is set it should be removed from the vector. 

class gameObject
{
protected:
    bool toBeDestroyed;
public:
    bool getDestroyed();
    void markToDestroy();
};

Now the first destroyed object gets removed from the vector successfully and then I get get an error that iterator is not dereferencable, pointing to the vector library at line 73(?).

I then check with the msvc debugger. In the data preview it shows that iterator points to the last/newest element of gameObjects. It is then removed (erase(it)) and AFTERWARDS the data preview doesn't change and calling it->getDestroyed() results in the error message.

Debug assertion failed! vector iterator not dereferencible.

PS: I checked cplusplus.com and vector.erase should return a new, valid iterator so I'm not sure where I'm messing it up.

€: After I was told about the erase-remove idiom I went ahead and ended up with the following, which doesn't compile. Due to my function being a member of gameObject I'm not sure how to successfully call remove_if. Thanks

gameObjects.erase(remove_if(gameObjects.begin(), gameObjects.end(), gameObject::getDestroyed), gameObjects.end());

€2: A lot of you pointed out the first object isn't being checked. I propably should've pointed that out but the first element is ALWAYS the player and shouldn't be removed. Thanks for your comments nevertheless. I'll try with a simple forward loop without getting too fancy ^^.

€3: I tried Jonathan Mees suggested code but I get the exact same error message. I'll try and find out where exactly it happens but I can't just put a breakpoint into the erasing part anymore. Will mess around a bit.

€4: Problem was solved by removing the else {} condition and always decrementing the iterator. Thanks again for all your replies.

Upvotes: 4

Views: 2445

Answers (3)

Jonathan Mee
Jonathan Mee

Reputation: 38969

vector::erase returns the:

Iterator following the last removed element. If the iterator pos refers to the last element, the end() iterator is returned.

Meaning that vector::erase will never return vector::begin (unless you removed the only element in the container.) So it will always be dereferenced again after vector::erase is called. It will be dereferenced even if vector::end was returned by the call to vector::erase which is of course illegal.

Instead of this loop, consider using remove_if which is designed for this purpose:

gameObjects.erase(remove_if(begin(gameObjects),
                            end(gameObjects),
                            [](const auto& i){ return i.getDestroyed(); }), end(gameObjects));

EDIT:

I noticed you try to use this in your edit. You cannot use a bare function pointer as the predicate. If you want to avoid a lambda, you should consider the use of mem_fn:

gameObjects.erase(remove_if(begin(gameObjects),
                            end(gameObjects),
                            mem_fn(&gameObject::getDestroyed)), end(gameObjects));

Live Example

If there's difficulty in reading that line feel free to use as many variable as you like:

auto p = mem_fn(&gameObject::getDestroyed);
auto result = remove_if(begin(gameObjects), end(gameObjects), p);

gameObjects.erase(result, end(gameObjects));

Upvotes: 3

UKMonkey
UKMonkey

Reputation: 7013

Your loop is a little messed up - you're iterating backwards, ignoring the first element, and testing some elements multiple times. Might I suggest rbegin() as an alternative?

Upvotes: 3

Dark Falcon
Dark Falcon

Reputation: 44201

Let's say you have 2 objects in your vector and the last one is is marked as destroyed. When you call erase, it will return a new, valid iterator pointing at the element after the erased element. There is no element after the erased element, so the returned iterator is gameObjects.end(). You then continue to the top of the loop and dereference this iterator, which is not valid. You need to decrement your iterator after the erase if you want it pointing at a valid element.

One other note: If you ever wanted your first element removed, it will not be. Your loop exits when the iterator == gameObjects.begin(), so the first element is never checked.

Is there some reason you wanted to do this in reverse? If there is no specific reason, I would recommend you use the method recommended by @Borgleader.

Upvotes: 5

Related Questions