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.

Answer 1

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.

Answer 2

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;
    }
  }
}