CODEWITHSUNDEEP
c++c++11
It'sPete
It'sPete

Reputation: 5221

std::multimap::erase() while iterating

I have a std::multimap that maps session IDs (ints) to the pieces of hardware used in that session (the hardware is described by a struct that contains some hardware specific info).

I have a cleanup function that has to do specific cleanup stuff for each piece of hardware. After the cleanup is done, I need to erase that element from the map, since the hardware is no longer used in that session.

Note that I don't just want to remove a single piece of hardware from a session. Instead the whole session gets torn down, so I want to search the map for the session ID, cleanup the hardware, and remove all those entries from the map.

Here's some code which shows what I am trying to explain:

void MyClass::end_session(const int session_id) {
  // session_map_ is a member variable of MyClass
  const auto range = session_map_.equal_range(session_id);
  for (auto it = range.first; it != range.second; session_map_.erase(it++)) {
    // do cleanup for the hardware pointed to by it->second
  }
}

Is the loop legal? I know that the iterator passed in to erase() gets invalidated, but this doesn't invalidate range.first or range.second, correct? Also, does session_map_.erase(it++) work like I would expect it to? That is, I'm assuming that it gets saved as the argument for erase(), gets incremented to its new value, and then erase() is called for the old value (thus invalidating the iterator before the increment). Is that correct?

Upvotes: 2

Views: 583

Answers (2)

Yakk - Adam Nevraumont
Yakk - Adam Nevraumont

Reputation: 275976

What you are doing is perfectly safe.

I, however, am leery of complex control flow in business logic. It is too hard to understand at a glance, and comments have a risk of becoming out of sync.

As such I write verbs.

template<class C, class It, class F>
void erase_remove_if( C& c, It b, It e, F&& f) {
  auto it = b;
  while (it != e) {
    if (f(*it))
      it = c.erase(it);
    else
      ++it
  }
}
template<class C, class F>
void erase_remove_if( C& c, F&& f) {
  using std::begin; using std::end;
  return erase_remove_if( c, begin(c), end(c), std::forward<F>(f) );
}

now your code reads like:

void MyClass::end_session(const int session_id) {
  // session_map_ is a member variable of MyClass
  const auto range = session_map_.equal_range(session_id);

  // remove said elements after doing some processing:
  erase_remove_if( session_map_, range.first, range.second, [&](auto& kv) {
    auto&& [key, elem] = kv;
    // do cleanup for the hardware pointed to by elem
    return true; // erase element
  });
}

basically fenceposting errors are so common and hard to prevent that I'd rather have my relatively complex loop control code in one spot.

Upvotes: 1

Pavan Chandaka
Pavan Chandaka

Reputation: 12831

The documentation says

Other references and iterators are not affected

So your loop runs as expected and erase works as expected.

Suppose you have 4 elements, in your case the loop runs 4 times without any problem.

https://en.cppreference.com/w/cpp/container/multimap/erase

Upvotes: 2

Related Questions