question about std::vector::end()

Question

I recently finished fixing a bug in the following function, and the answer surprised me. I have the following function (written as it was before I found the bug):

    void Level::getItemsAt(vector<item::Item>& vect, const Point& pt)
    {
        vector<itemPtr>::iterator it; // itemPtr is a typedef for a std::tr1::shared_ptr<item::Item>
        for(it=items.begin(); it!=items.end(); ++it)
        {
            if((*it)->getPosition() == pt)
            {
                item::Item item(**it);
                items.erase(it);
                vect.push_back(item);
            }
        }
    }

This function finds all Item objects in the 'items' vector that has a certain position, removes them from 'items', and puts them in 'vect'. Later, a function named putItemsAt does the opposite, and adds items to 'items'. The first time through, getItemsAt works fine. After putItemsAt is called, though, the for loop in getItemsAt will run off the end of 'items'. 'it' will point at an invalid Item pointer, and getPosition() segfaults. On a hunch, I changed it!=items.end() to it<items.end(), and it worked. Can anyone tell me why? Looking around SO suggests it might involve erase invalidating the iterator, but it still doesn't make sense why it would work the first time through.

I'm also curious because I plan to change 'items' from a vector to a list, since list's erase is more efficient. I know I'd have to use != for a list, as it doesn't have a < operator. Would I run into the same problem using a list?

Answer 1

You're invoking undefined behavior. All the iterators to a vector are invalidated by the fact that you called erase on that vector. It's perfectly valid for an implementation to do whatever it wants.

When you call items.erase(it);, it is now invalid. To conform to the standard, you must now assume that it is dead.

You invoke undefined behavior by using that invalid iterator in the next call to vect.push_back.

You invoke undefined behavior again by using it as the tracking variable of your for loop.

You can make your code valid by using std::remove_copy_if.

class ItemIsAtPoint : std::unary_function<bool, item::Item>
{
    Point pt;
public:
    ItemIsAtPoint(const Point& inPt) : pt(inPt) {}
    bool operator()(const item::Item* input)
    {
        return input->GetPosition() == pt;
    }
};

void Level::getItemsAt(vector<item::Item>& vect, const Point& pt)
{
    std::size_t oldSize = items.size();
    std::remove_copy_if(items.begin(), items.end(), std::back_inserter(vect), 
        ItemIsAtPoint(pt));
    items.resize(vect.size() - (items.size() - oldSize));
}

You can make this a lot prettier if you are using boost::bind, but this works.

Answer 2

I'll go with Remy Lebeau's explanation about iterator invalidation, and just add that you can make your code valid and asymptotically faster (linear time, instead of quadratic time) by using a std::list instead of a std::vector. (std::list deletions only invalidate the iterator that was deleted, and insertions don't invalidate any iterators.)

You can also predictibly identify iterator invalidation while debugging by activating your STL implementation's debug mode. On GCC, you do with with the compiler flag -D_GLIBCXX_DEBUG (see some caveats there).

Answer 3

When you call erase(), that iterator becomes invalidated. Since that is your loop iterator, calling the '++' operator on it after invalidating it is undefined behavor. erase() returns a new valid iterator that points to the next item in the vector. You need to use that new iterator from that point onwards in your loop, ie:

void Level::getItemsAt(vector<item::Item>& vect, const Point& pt) 
{ 
    vector<itemPtr>::iterator it = items.begin();
    while( it != items.end() )
    {
        if( (*it)->getPosition() == pt )
        {
            item::Item item(**it);
            it = items.erase(it);
            vect.push_back(item);
        }
        else
            ++it;
    } 
}