1

I already went through this post Deleting elements from STL set while iterating

Still, I want to understand why the code below produces the wrong result.

int main() {
    unordered_set<int> adjacency;
    adjacency.insert(1);
    adjacency.insert(2);

    for (const auto& n : adjacency) {
        adjacency.erase(n);
    }

    cout <<"After removing all elements: " << endl;
    for (const auto& n : adjacency) {
        cout << n << " ";
    }
    cout << endl;

    return 0;
}

The adjacency contains 1 and 2. After erasing all elements through for-loop, it still contains element 1. Why?

I am using version (2) erase function below, so the rule "Versions (1) and (3) return an iterator pointing to the position immediately following the last of the elements erased." does not apply?

UPDATE: the reason of not using clear() is that it requires removing the element one by one to do some other processing.

by position (1) 
iterator erase ( const_iterator position );
by key (2)  
size_type erase ( const key_type& k );
range (3)   
iterator erase ( const_iterator first, const_iterator last );

Version (2) returns the number of elements erased, which in unordered_set containers (that have unique values), this is 1 if an element with a value of k existed (and thus was subsequently erased), and zero otherwise.

Versions (1) and (3) return an iterator pointing to the position immediately following the last of the elements erased.

Thanks!

thinkdeep
  • 945
  • 1
  • 14
  • 32
  • If you're going to erase all elements from the set, why not simple [clear it](http://en.cppreference.com/w/cpp/container/unordered_set/clear) after the loop? – Some programmer dude Sep 12 '17 at 13:23
  • I updated the question – thinkdeep Sep 12 '17 at 13:26
  • 3
    "[the iterators and references to the elements removed are invalidated](http://www.cplusplus.com/reference/unordered_set/unordered_set/erase/)." Apply this rule to your situation: The code uses an invalid iterator. The behavior is undefined. – Raymond Chen Sep 12 '17 at 13:34
  • Question updated. The rule seems not to be applicable here? – thinkdeep Sep 12 '17 at 13:42
  • 3
    range-based for-loops use iterators under the hood (see [this](http://en.cppreference.com/w/cpp/language/range-for)), so it is applicable here – sp2danny Sep 12 '17 at 13:48

2 Answers2

4

Range-based for-loops use iterators under the hood, so what you wrote leads to undefined behaviour.

If you need to process all elements, and then remove some of them based on some criteria, there is a way to do that that works on all containers:

for(auto it = adjacency.begin(); it != adjacency.end();)
{
    Process(*it);
    if (Condition(*it))
        it = adjacency.erase(it);
    else
        ++it;
}

If you need to process all items, and then remove all, then do that:

std::for_each(adjacency.begin(), adjacency.end(), &Process);
adjacency.clear();
sp2danny
  • 7,488
  • 3
  • 31
  • 53
2

You are pulling the rug out from underneath your own feet, as Raymond pointed out.

#include <iostream>
#include <unordered_set>

using namespace std;

int main()
{
    typedef unordered_set<int> adjacency_t;
    typedef adjacency_t::iterator adjacencyIt_t;
    adjacency_t adjacency;
    adjacency.insert(1);
    adjacency.insert(2);

    cout <<"Before: " << endl;
    for (const auto& n : adjacency) {
        cout << n << " ";
    }
    cout << endl;

    for (adjacencyIt_t i = adjacency.begin(); i!=adjacency.end(); /*empty*/)
    {
        // Do some processing on *i here.
        adjacency.erase(i++); // Don't erase the old iterator before using it to move to the next in line.

    }

    cout <<"After removing all elements: " << endl;
    for (const auto& n : adjacency) {
        cout << n << " ";
    }
    cout << endl;

    return 0;
}
Steve
  • 1,760
  • 10
  • 18