How to iterate over a STL set and selectively remove elements?

Question

The following code does not work correctly. How should it be done correctly?

for (std::set<Color>::iterator i = myColorContainer.begin();
            i!=myColorContainer.end();
            ++i)
{
    if ( *i  == Yellow)
    {
        DoSomeProccessing( *i );
        myColorContainer.erase(i);
    }
}

Solution 1

You don't need a loop as youre dealing with a set.

std::set<Color>::iterator it = myColorContainer.find(Yellow);
if (it != it.myColorContainer.end()){
  DoSomeProcessing(*it);
  myColorContainer.erase(it);
}

Solution 2

Try:

for(std::set<Color>::iterator it = myColorContainer.begin(); 
    it != myColorContainer.end();) { // note missing it++
     if( (*it) == Yellow ) {
        DoSomeProcessing(*it);
        myColorContainer.erase(it++); // post increment (original sent to erase)
     }
     else {
       ++it; // more efficient than it++;
     }
}

Solution 3

for (std::set<Color>::iterator i = myColorContainer.begin();
            i!=myColorContainer.end(); /* No i++ */)
{
    if ( *i  == Yellow)
    {
        DoSomeProccessing( *i );
        std::set<Color>::iterator tmp = i;
        ++i;
        myColorContainer.erase(tmp);
    }
    else {
        ++i;
    }
}

Once you go to next message with ++i it is guaranteed to be valid - property of std::set that iterators on inserted elements are never invalidated unless the element is removed.

So now you can safely erase previous entry.

Comments

[Patrick]

This won't work either. You should assign the return value of erase to it again.

[Adrian Regan]

The returned iterator is a microsoft specific implementation that breaks the standard : msdn.microsoft.com/en-us/library/8h4a3515%28VS.80%29.aspx. Sure enough, you need to increment the iterator after the erase.

[Adrian Regan]

code is standards compliant. I agree @Viktor Sehr this would be the preferred way to remove an element from the set. However, the question asks how to get the code snippet to work.

[Viktor Sehr]

@daramarak:I think you answered while I edited the code (thought it was a std::vector in my first post)

[scippie]

This sollution is perfect if you can't use the m$-specific implementation and need to use a loop. If you don't need to use a loop, Viktor's option is even better. Thanks for a great answer. You've been a great help.