std::map and remove_if

[Ceset]

Hi guys.

Recently i am trying to write a game state manager. And i failed to create a function that will remove all states except one.

statemanager.h

class StatesManager
{
public:
    StatesManager();

    .
    .
    .

    void removeAllStatesExcept(StateID identifier);

private:
    std::map<StateID, std::unique_ptr<State>> mStates;
};

StateID is an enum class.

And here is statemanager.cpp

.
.
void StatesManager::removeAllStatesExcept(StateID identifier)
{
    mStates.erase(
                std::remove_if(mStates.begin(), mStates.end(),
                               [&] (const std::pair<StateID, std::unique_ptr<State>> p) 
                                    { return std::get<0>(p) != identifier; } )
                  , mStates.end());
}
.
.

And the compiler gives

/usr/include/c++/4.9.0/bits/predefined_ops.h:231: error: cannot bind 'std::pair<const StateID, std::unique_ptr<State> >' lvalue to 'std::pair<const StateID, std::unique_ptr<State> >&&'
  { return bool(_M_pred(*__it)); }
                              ^

/usr/include/c++/4.9.0/bits/stl_pair.h:170: error: assignment of read-only member 'std::pair<const StateID, std::unique_ptr<State> >::first'
  first = std::forward<first_type>(__p.first);
        ^

I am not well acquainted with c++11 features yet. Which confuses me about where i am doing wrong.

Thanks in advance.

[Peter87]

It's not possible to use std::remove_if on associative containers like std::map.

[Ceset]

Thanks a lot for that valuable knowledge @Peter87.

And if anyone ever desires i changed the code to this. Hope this helps others like me.

    for(std::pair<const StateID, std::unique_ptr<State>>& p : mStates)
    {
        if(p.first != identifier)
            mStates.erase(p.first);
    }

[Cubbi]

That's wrong: by erasing p.first, you're invalidating the iterator that the range-for loop is going to increment next step.

Use the usual iterator-based erase loop

for( auto it = mStates.begin(); it != mStates.end(); ) {
    if( it->first != identifier ) it = mStates.erase(it);
    else ++it;
}

[Ceset]

Thanks for the warning @Cubbi. I changed the code.

It is now

    for(std::map<StateID, std::unique_ptr<State>>::iterator itr = mStates.begin(); itr != mStates.end(); )
    {
        if( itr->first != identifier )
            itr = mStates.erase(itr);
        else ++itr;
    }

Didn't use auto just to be aware of it is an std::map iterator.

[JLBorges]

> create a function that will remove all states except one.

If the size of the map is not very small, this would be more efficient:

void StatesManager::removeAllStatesExcept(StateID identifier)
{
     auto iter = mStates.find(identifier) ;
     if( iter != mStates.end() )
     {
          const auto pair = *iter ; // make a copy
          mStates.clear() ; // remove all entries
          mStates.insert( pair ) ; // insert the copy
      } 
}

[Ceset]

But i designed this as a game state manager. So most probably more than 3 states will not exist at the same time. At least for my usage purposes.

Thanks for the help though. I will consider this possibility if i ever make a container with lots of elements.