unordered_map not being updated properly

user_13321620 0
Viewed 141

Trying to update an unordered map using the following code snippet to have only lowercase letters, but it seems to stop after erasing one key-value pair { [33 '!']: 3 } and exits the loop leaving the rest of the map unvisited and prints the partly updated map.

 for (auto &i : m)
        if (!(i.first >= 'a' && i.first <= 'z'))
            m.erase(i.first);

Following debugging images revealed the above

enter image description here

enter image description here

The complete code is herewith:

#include <iostream>
#include <unordered_map>
#include <algorithm>    
using namespace std;
int main()
{
    string line = "Try! Try! Try! until you succeed";
    //getline(cin, line);
    unordered_map<char, int> m;
    for (int i = 0; line[i]; i++)
    {   
        char lower = (char)tolower(line[i]);
        if (m.find(lower) == m.end())
            m.insert(make_pair(lower, 1));
        else
            m[lower]++;
    }

    for (auto &i : m) //only updates until ! 
        if (!(i.first >= 'a' && i.first <= 'z'))
            m.erase(i.first);

    cout<<"The freq. map so formed is : \n";
    for (auto &i : m)
        cout<<i.first<<"\t"<<i.second<<endl;
    
    return 0;
}
/*
OUTPUT : 
The freq. map so formed is : 
d       1
t       4
r       3
e       2
y       4
l       1
o       1
        5
n       1
u       3
i       1
s       1
c       2
*/

Can't seem to understand why it won't loop through the complete unordered map.

Also, not sure if this helps towards a clear picture but, when the standard map is used instead of unordered map it gives an Address Boundary Error at the same instance where the next character of the map needs to be updated like so:

enter image description here

enter image description here

unordered-map segmentation-fault c++
2 Answers

You cannot erase the element of a map while iterating this way. When you erase the iterator, it becomes invalidated, so you need to explicitly increment it before you delete the element.

Try this code instead:

 for (auto it = m.begin(); it != m.end();)
     if (!((*it).first >= 'a' && (*it).first <= 'z'))
         it = m.erase(it);
     else
        ++it;
user_168426490

One of the c++ traps is that iterators are invalidated for most containers when they are modified.

std::unordered_map<Key,T,Hash,KeyEqual,Allocator>::erase - cppreference.com

References and iterators to the erased elements are invalidated. Other iterators and references are not invalidated.

So when you have remove item form m current iterators are become invalid.

Now range base for loop uses iterators underneath.

The beast way to fix it us use std::erase_if algorithm:

std::erase_if(m.begin(), m.end(), [](const auto& i) { 
    return !(std::islower(i.first)); 
});
user_13874380
Related