Vector iterator not dereferencable?

问题:

I'm getting that error with this code:

for(std::vector<AguiTimedEvent*>::iterator it = timedEvents.begin();
    it != timedEvents.end();)
{
    if((*it)->expired())
    {
        (*it)->timedEventCallback();
        delete (*it);

        it = timedEvents.erase(it);
    }
    else
        it++;
}

What could be the problem?

the timed event sometimes pushes a new one in when its callback is called, that might do it

Thanks

回答1:

If you are looping through a vector and the callback function causes the vector to be added to, then all iterators into the vector may be invalidated including the loop variable it.

In this case (where the callback modifies the vector) you are probably better off using an index as your loop variable.

You probably need to do some thorough analysis of the design to make sure that you aren't going to create any unexpected infinite loops.

E.g.

for(std::vector<AguiTimedEvent*>::size_type n = 0;
    n < timedEvents.size();)
{
    if(timedEvents[n]->expired())
    {
        timedEvents[n]->timedEventCallback();
        delete timedEvents[n];

        timedEvents.erase(timedEvents.begin() + n);
    }
    else
        n++;
}

回答2:

the timed event sometimes pushes a new one in when its callback is called, that might do it

Yes.

If you add an element to the vector, it may be invalidated.

回答3:

Edit:

the timed event sometimes pushes a new one in when its callback is called, that might do it

Yes, that is very definitely unsafe. If the vector has to resize, then all of it's pointers and iterators would be invalidated. You should never, ever insert into the middle of a vector whilst it's being iterated through, that will cause death. Maybe if you converted to a numerical for loop, the problem would be solved.

回答4:

Sounds like a job for std::remove_copy_if and std::remove_if: (Haven't tested this but it should give you the idea...)

#include <algorithm>
#include <functional>

//I'm sure you probably already have a delete functor lying around, right?
// No? Well here's one for you....
struct Deleter : public std::unary_function<AguiTimedEvent*, void>
{
    void operator()(AguiTimedEvent* toNuke)
    {
        delete toNuke;
    }
};

std::vector<AguiTimedEvent*> toRun;
std::remove_copy_if(timedEvents.begin(), timedEvents.end(), 
    std::back_inserter(toRun), std::not1(std::mem_fun(&AguiTimedEvent::expired)));
timedEvents.erase(std::remove_if(timedEvents.begin(), timedEvents.end(),
    std::mem_fun(&AguiTimedEvent::expired), timedEvents.end());
std::for_each(toRun.begin(), toRun.end(), 
    std::mem_fun(&AguiTimedEvent::timedEventCallback));
std::for_each(toRun.begin(), toRun.end(), Deleter());

Note that this solution takes linear time, while yours takes quadradic time. This also cleanly gets rid of the issue that the callbacks might add to the new vector by removing the decisions about that until after the removal from the vector has already been done. On the other hand, it checks the expired flag twice, so if that's a complex operation this might be slower.