35

The proper way to iterate is to use iterators. However, I think by erasing, the iterator is invalidated.

Basically what I want to do is:

for(iterator it = begin; it != end; ++it)
{
    if(it->somecondition() )
    {
     erase it
    }

}

How could I do this without v[i] method?

Thanks

struct RemoveTimedEvent
{
    bool operator()(const AguiTimedEvent& pX, AguiWidgetBase* widget) const 
    {
        return pX.getCaller() == widget;
    }
};

void AguiWidgetContainer::clearTimedEvents( AguiWidgetBase* widget )
{
    std::vector<AguiTimedEvent>::iterator it = std::remove_if(timedEvents.begin(),
        timedEvents.end(), RemoveTimedEvent());
    timedEvents.erase(it, timedEvents.end());

}
jmasterx
  • 52,639
  • 96
  • 311
  • 557

1 Answers1

57

erase() returns a new iterator:

for(iterator it = begin; it != end(container) /* !!! */;)
{
    if (it->somecondition())
    {
        it = vec.erase(it);  // Returns the new iterator to continue from.
    }
    else
    {
        ++it;
    }
}

Note that we can no longer compare it against a precalculated end, because we may erase it and therefore invalidate it. We must get the end explicitly each time.

A better method might be to combine std::remove_if and erase(). You change from being O(N2) (every element gets erased and shifted as you go) to O(N):

iterator it = std::remove_if(begin, end, pred);
vec.erase(it, vec.end());

Where pred is your removal predicate, such as:

struct predicate // do choose a better name
{
    bool operator()(const T& pX) const // replace T with your type
    {
        return pX.shouldIBeRemoved();
    }
};

iterator it = std::remove_if(begin, end, predicate());
vec.erase(it, vec.end());

In your case, you can make it pretty general:

class remove_by_caller
{
public:
    remove_by_caller(AguiWidgetBase* pWidget) :
    mWidget(pWidget)
    {}

    // if every thing that has getCaller has a base, use that instead
    template <typename T> // for now a template
    bool operator()(const T& pX) const
    {
        return pX.getCaller() == mWidget;
    }

private:
    AguiWidgetBase* mWidget;
};

std::vector<AguiTimedEvent>::iterator it =
    std::remove_if(timedEvents.begin(), timedEvents.end(), remove_by_caller(widget));
timedEvents.erase(it, timedEvents.end());

Note lambda's exist to simplify this process, both in Boost and C++11.

GManNickG
  • 494,350
  • 52
  • 494
  • 543
  • I'm not sure I understand how the first method is N squared, why is it not N, I only iterate through once to remove the items. – jmasterx Oct 15 '10 at 01:49
  • Unless ofcourse the iterator returned by erase brings you back to the top – jmasterx Oct 15 '10 at 01:50
  • The problem with that is the condition is dependent on an argument of a function passed to the erase function – jmasterx Oct 15 '10 at 01:52
  • 1
    @Milo: It's N^2. If you erase the first element, all the other elements will be copied down to take it's place. Do that N times and you've touched each one N^2 times. You can pass the argument to the predicate, which forwards it. – GManNickG Oct 15 '10 at 01:54
  • @GMan, I'm still not sure how I can pass an additional argument to the () operator. see my edit: – jmasterx Oct 15 '10 at 02:11
  • 1
    The first snippet is perhaps wrong as the `end` iterator would've been invalidated after the erasure. – legends2k Dec 31 '14 at 11:07
  • @legends2k: Nice catch on an old answer. Fixed. – GManNickG Dec 31 '14 at 21:37
  • 6
    I would not re-evaluate `end()` on every loop iteration, just when `erase()` is called: `for(iterator it = vec.begin(), end = vec.end(); it != end;) { ... if (condition) { it = vec.erase(it); end = vec.end(); } ... }` – Remy Lebeau Dec 31 '14 at 23:28