C++ Segmentation when using erase on std::list

Question

I'm trying to remove items from a C++ linked list using erase and a list iterator:

#include <iostream>
#include <string>
#include <list>

class Item
{
  public:
    Item() {}
    ~Item() {}
};

typedef std::list<Item> list_item_t;


int main(int argc, const char *argv[])
{

  // create a list and add items
  list_item_t newlist;
  for ( int i = 0 ; i < 10 ; ++i )
  {
    Item temp;
    newlist.push_back(temp);
    std::cout << "added item #" << i << std::endl;
  }

  // delete some items
  int count = 0;
  list_item_t::iterator it;

  for ( it = newlist.begin(); count < 5 ; ++it )
  {
    std::cout << "round #" << count << std::endl;
    newlist.erase( it );
    ++count;
  }
  return 0;
}

I get this output and can't seem to trace the reason:

added item #0
added item #1
added item #2
added item #3
added item #4
added item #5
added item #6
added item #7
added item #8
added item #9
round #0
round #1
Segmentation fault

I'm probably doing it wrong, but would appreciate help anyway. thanks.

Answer 1

When you erase an element at position it, the iterator it gets invalidated - it points to a piece of memory that you just freed.

The erase(it) function returns another iterator pointing to the next element to the list. Use that one!

Answer 2

The core problem here is you're using at iterator value, it, after you've called erase on it. The erase method invalidates the iterator and hence continuing to use it results in bad behavior. Instead you want to use the return of erase to get the next valid iterator after the erased value.

it = newList.begin();
for (int i = 0; i < 5; i++) {
  it = newList.erase(it);
}

It also doesn't hurt to include a check for newList.end() to account for the case where there aren't at least 5 elements in the list.

it = newList.begin();
for (int i = 0; i < 5 && it != newList.end(); i++) {
  it = newList.erase(it);
}

As Tim pointed out, here's a great reference for erase

• http://www.cplusplus.com/reference/stl/list/erase/

Answer 3

You're invalidating your iterator when you erase() within the loop. It would be simpler to do something like this in place of your erase loop:

list_item_t::iterator endIter = newlist.begin();
std::advance(endIter, 5);
newList.erase(newlist.begin(), endIter);

You might also be interested in the erase-remove idiom.

Answer 4

I do this:

for(list<type>::iterator i = list.begin(); i != list.end(); i++)
{
     if(shouldErase)
     { 
        i = list.erase(i);
        i--;
     }
}

Edited because I'm a bonehead that can't read apparently lol.