Weird seg fault when erasing from a map

Question

I have the following code:

//update it in the map
      std::map<std::string, std::string>::iterator it;
      for(it = spreadsheets.at(i).cells.begin(); it != spreadsheets.at(i).cells.end(); ++it)
      {
        if(it->first == change.first)
        {
          if(change.second == "")
          {
            spreadsheets.at(i).cells.erase(change.first);
          }
          else
          {
          it->second = change.second;
          }
        }
      }

The code above runs perfectly on my mac however when I run in on a linux computer it throws a seg fault on spreadsheets.at(i).cells.erase(change.first);

Any idea whats causing this error? Ive tried changing erase(change.first) to erase(it) and I am still getting the seg fault.

W.B. · Answer 1 · 2013-04-23T21:12:50.930

Because when you erase from the container, your iterator is no longer valid, yet your loop continues.

You could change your loop to:

std::map<std::string, std::string>::iterator it = spreadsheets.at(i).cells.begin();
while (it != spreadsheets.at(i).cells.end())
{
  if(it->first == change.first)
  {
    if(change.second == "")
    {
      spreadsheets.at(i).cells.erase(it++);  //Post increment returns a copy pointing at the current element, while it already points to the next element and thus stays valid after erase
    }
    else
    {
      it->second = change.second;
      ++it;
    }
  }
  else
    ++it;
}

Now that I think of it, why do you erase with first element of the pair the iterator is pointing to, ie:

spreadsheets.at(i).cells.erase(change.first);

instead of

spreadsheets.at(i).cells.erase(it);

It's less efficient, as another lookup has to be made.

syam · Accepted Answer · 2013-04-23T21:08:35.737

From the documentation of std::map::erase:

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

Still your loop goes on and you increment your (now invalid) iterator.

Fix: increment your iterator another way, eg.:

std::map<std::string, std::string>::iterator it;
for (it = spreadsheets.at(i).cells.begin(); it != spreadsheets.at(i).cells.end();/*NOTE: no increment here*/)
{
  if (it->first == change.first)
  {
    if (change.second == "")
    {
      it = spreadsheets.at(i).cells.erase(it); // C++11 only
      // in both C++03 and C++11:  spreadsheets.at(i).cells.erase(it++);
    }
    else
    {
      it->second = change.second;
      ++it;
    }
  }
  else
    ++it;
}

Or to avoid confusion because of the numerous execution paths (the very same confusion that made me forget the last else on my first try): just copy the iterator, increment the original one, and then use the copy. This may look overkill in your case but for more complex loops this is sometimes the only way to stay sane. ;)

std::map<std::string, std::string>::iterator it;
for (it = spreadsheets.at(i).cells.begin(); it != spreadsheets.at(i).cells.end();/*NOTE: no increment here*/)
{
  std::map<std::string, std::string>::iterator this_it = it++;
  if (this_it->first == change.first)
  {
    if (change.second == "")
    {
      spreadsheets.at(i).cells.erase(this_it);
    }
    else
    {
      this_it->second = change.second;
    }
  }
}

@Deekor: incrementing an invalidated iterator is Undefined Behaviour, which means basically "anything could happen" (it could even have the *appearance* of working, which is what happens on your Mac). — syam, Apr 23 '13 at 20:58
@Deekor: I updated my code, there was a path where I forgot to increment the iterator (the last `else`). Be sure to double-check it or you will have a bug. — syam, Apr 23 '13 at 21:01
@syam, in C++03 it's enough to simply put `spreadsheets.at(i).cells.erase(it++);` as post increment returns a copy of the iterator. — W.B., Apr 23 '13 at 21:02
Thanks for the detailed answer. Because I only needed to update one item I was able to just 'break' after the 'erase' — Deekor, Apr 24 '13 at 02:37

score 2 · Answer 3 · edited May 23 '17 at 12:06

2

After element being erased from map pointing to this element will become invalidated. Thus spreadsheets.at(i).cells.erase(change.first); renders it invalid. See Iterator Invalidation Rules

edited May 23 '17 at 12:06

Community

1
1

answered Apr 23 '13 at 20:53

alexrider

4,419
1
15
27

score 1 · Answer 4 · answered Sep 02 '15 at 09:47

References and iterators to the erased elements are invalidated.

//update it in the map
std::map<std::string, std::string>::iterator it;
for(it = spreadsheets.at(i).cells.begin(); it != spreadsheets.at(i).cells.end(); ++it)
{
    if(it->first == change.first)
    {
        if(change.second == "")
        {
            spreadsheets.at(i).cells.erase(it--);
        }
        else
        {
            it->second = change.second;
        }
    }
}

score 0 · Answer 5 · edited May 23 '17 at 11:58

0

At the moment you do spreadsheets.at(i).cells.erase(change.first); the iterator in the std::map (at the current change.first key) is invalidated. So, when you do it++, it is undefined behaviour.

cf Rules for Iterator Invalidation for rules about invalidation of iterators in standard containers

edited May 23 '17 at 11:58

Community

1
1

answered Apr 23 '13 at 20:52

Stephane Rolland

34,892
31
111
159

score 0 · Answer 6 · edited May 23 '17 at 12:06

0

Increment the iterator before you erase it. Why didn't it happen on the Mac? Who knows.. different OS, different behaviour.

SO

edited May 23 '17 at 12:06

Community

1
1

answered Apr 23 '13 at 20:56

codah

426
4
13

Weird seg fault when erasing from a map

6 Answers6