C++ multimap iterator invalidation

Question

I'm trying to figure out how std::multimap iterators work, therefore I've created a simple example that shows the substance of my problem. If uncomment case 1, I expect iterator to point to the first element with the key 1, but in reality it prints all the values associated with key 0 (like nothing was erased) and sometimes it crashes, probably because iterator is invalid. However if uncomment case 2, all the values with key 1 are properly deleted.

Is there any way to know what is the next valid iterator for the multimap after erasure? (for example std::vector.erase(...) returns one)

std::multimap<int, int> m;

for(int j=0; j<3; ++j) {
    for(int i=0; i<5; ++i) {
        m.insert(std::make_pair(j, i));
    }
}

for(std::multimap<int, int>::iterator it=m.begin(); it!=m.end();) {
    printf("%d %d\n", (*it).first, (*it).second);
    ++it;
    if( (*it).second == 3 ) {
        //m.erase(0);     //case 1
        m.erase(1);     //case 2
    }
}

Answer 1

The cause of the problem

When you call m.erase(0) in you example, it points at an element with the key 0 - so it is invalidated. m.erase(1) works, because when it is called the first time, it is not pointing to an element with the key 1, so it is not affected. In later iterations, no elements with the key 1 remain, so nothing is deleted, and no iterator is affected.

The Solution

multimap does not have an erase-method that returns the next valid iterator. One alternative is to call it = m.upper_bound(deleted_key); after the deletion. This is logarithmic, though, which might be too slow for your scenario (erase(x) and upper_bound would be two logarithmic operations).

Assuming you want to erase the key your iterator is currently pointing to, you could do something like this (otherwise, erase is fine, of course; not tested):

std::multimap<int, int>::iterator interval_start = m.begin();
for(std::multimap<int, int>::iterator it=m.begin(); it!=m.end(); ++it) {
    if(interval_start->first < it->first) // new interval starts here
        interval_start == it;
    if( (*it).second == 3 ) {
        std::multimap<int, int>::iterator interval_end = it;
        while((interval_end != m.end()) && (interval_end->first == it->first)) {
            ++interval_end; // search for end of interval - O(n)
        }
        m.erase(interval_start, interval_end); // erase interval - amortized O(1)
        it = interval_end; // set it to first iterator that was not erased
        interval_start = interval_end; // remember start of new interval
    }
}

This uses one linear operation, all the rest are constant time. If your map is very large, and you only have few items with equal keys, this will likely be faster. However, if you have many items with equal keys, the search for the end of the interval, is probably better done using upper_bound (O(log n) instead of O(n) when searching the end of the interval).

Answer 2

when you erase the iterator becomes invalid. instead remember the next element then erase:

std::map<int,int>::iterator next = m + 1;
m.erase
m = next;

Answer 3

First answer

std::multimap<int, int> m;
//   ^^^^^^^^
std::map<int, int>::iterator it=m.begin(); 
//   ^^^

Hum....

Second answer, re: edited question

for(std::multimap<int, int>::iterator it=m.begin(); it!=m.end();) {
    .... stuff ....
        m.erase(1); // container mutation
    .... stuff ....
}

Be extremely careful when you are mutating a container (any container) when you are iterating on it, as you might invalidate an iterator you depend on.

The so-called "node-based containers" (list, set, map...) are the most robust container WRT iterator invalidation: they only invalidate iterators to deleted elements (there is no way for these iterators not be invalidated).

In this case you should check that the element you are about to delete isn't actually *it.

I am not quite sure what you are trying really to do with your loop.

Answer 4

From looking at your code, I think that your ++it is causing the problem. You are assigning it to a place that might have been deleted. move it to the end, after the if statement and test. like so:

for(std::multimap<int, int>::iterator it=m.begin(); it!=m.end();) {
    printf("%d %d\n", (*it).first, (*it).second);
    if( (*it).second == 3 ) {
        //m.erase(0);     //case 1
        m.erase(1);     //case 2
    }
    ++it;
}

Answer 5

(Edited)

for(std::multimap<int, int>::iterator it=m.begin(); it!=m.end();) {
    printf("%d %d\n", (*it).first, (*it).second);
    ++it;
    if( (*it).second == 3 ) {
        //m.erase(0);     //case 1
        m.erase(1);     //case 2
    }
}

In addition to invalidation of it iterator due to m.erase that may occur depending on the contents of multimap (already covered in another answer) there is always the problem that you dereference m.end() iterator on the last iteration of your for loop when you do if( (*it).second == 3 ) each time you run your program.

I suggest to run and debug with debug builds. I'm almost sure that every sane standard library implementation should contain assert to detect end() dereferencing.

Answer 6

Some guys above already have answered that you are playing with a fire.

Also, I think you are forgetting that multimap is ordered map, so you are iterating from the smallest keys to the largest ones. Therefore in the first case you remove keys after printing some of them, but in the second case you are remove just before going to them.