3

I don't understand, why it is runtime error? Erase set element while iterating.

set<int> sset;
sset.insert(3);
sset.insert(5);
sset.insert(6);

for(auto s: sset){
    sset.erase(s);
}

2 Answers2

4

Sadly, erasing elements from sset will invalidate the implicit iteration over the container; that's used by the nice syntactic sugar auto s : sset.

That's undefined behaviour.

Bathsheba
  • 231,907
  • 34
  • 361
  • 483
4

So just to explain further,

What you effectively have written is:

for (set<int>::const_iterator i=sset.begin(), e=sset.end(); i != e; i++)
{
    auto s = *i;
    sset.erase(s);
}

So the issue is that on doing the erase, the internal iterator i becomes invalidated. This is a general pain with trying to sequentially delete the content of many of the containers.

The following more traditional sequential delete code is also bad for the same reason, but perhaps more obviously:

for (set<int>::iterator i=sset.begin(), e=sset.end(); i != e; i++)
{
    sset.erase(i);
}

Fixes:

Generally, it is simpler to rely on context swap destruction of the whole container, when you can:

C++98: SsetType().swap(sset); 
C++11: sset = decltype<sset>();

And you could just do:

sset.erase(sset.begin(), sset.end());

Another way to fix this is to just keep deleting the begin() until the set is empty()

But the problem with all of these is you can't easily extend them to conditionally erase members of a set you are iterating through. Yes, there are helpers for conditional erase as well, and they can be used with lambdas, so they can carry state, but they generally tend to be as hard to use as rolling your own loop.

Since c++11, set::erase(iterator) returns a new iterator which is safe to continue iterating with, so you can write:

for (set<int>::iterator i=sset.begin(), e=sset.end(); i != e; )
{
    i = sset.erase(i);
}

If you were performing some conditional test, then:

for (set<int>::iterator i=sset.begin(), e=sset.end(); i != e; )
{
    if ( ... condition ... )
        i = sset.erase(i);
    else
        i++;
}

before, in c++98, you would have written something like:

for (set<int>::iterator i=sset.begin(), e=sset.end(); i != e; )
{
    auto j = i;
    j++;
    if ( ... condition ... )
        i = sset.erase(i);
    i = j;
}

As an exercise, you can roll the use of j into the for statement. getting the initial j++ in C98 is tricky, though!

Gem Taylor
  • 5,381
  • 1
  • 9
  • 27
  • Are you *sure* the first snippet should have a `const_iterator`? – Bathsheba Jul 06 '18 at 11:05
  • You should use `auto` – Mohit Jul 06 '18 at 11:07
  • @Mohit: Why?͏͏͏͏͏ – Bathsheba Jul 06 '18 at 11:07
  • @Bathsheba It'll automatically identify the type of the variable. You need not to type such a long type names. – Mohit Jul 06 '18 at 11:09
  • @Mohit: That's not necessarily a bad thing. Using `auto` in old-style `for` loops can get you into trouble if the type of an object is changed by an errant refactorer. – Bathsheba Jul 06 '18 at 11:10
  • 2
    @Bathsheba: anyway "`key`" cannot be modified so `const_iterator` and `iterator` are mostly equivalent for `std::set`. – Jarod42 Jul 06 '18 at 11:23
  • @Jarod42: Of course, you're correct. I'll upvote the question for good feelings and refrain from trolling for a while. – Bathsheba Jul 06 '18 at 11:26
  • I wanted to spell out exactly what was happening under the hood (well as close as is useful) so wanted to avoid auto. How would using auto have helped explain what was going on? As to const, yes, perhaps the true implementation does not use const so that it can support non-const-ref, but it seemed correct to the usage. As @Jarod42 says, it is irrelevant for set, anyway, just personal stylee. – Gem Taylor Jul 06 '18 at 12:39