std::list::erase not working

Question

I am trying to delete an element from a list of objects if one of the object's properties matches a condition. This is my function to do so, however, after performing this operation and then printing the contents, the erase() seems to have no effect. What am I doing wrong here?

void FileReader::DeleteProcess(int id, list<Process> listToDeleteFrom)
{
    list<Process>::iterator process;

    for(process = listToDeleteFrom.begin(); process != listToDeleteFrom.end(); process++)
    {
        if (process -> ID == id)
        {
            listToDeleteFrom.erase(process);
        }
    }
}

score 11 · Answer 1 · answered Oct 01 '12 at 02:31

First, you need to pass the list by reference; your code is working on a copy, so changes it makes won't affect the caller's list:

void FileReader::DeleteProcess(int id, list<Process> & listToDeleteFrom)
                                                     ^

Second, erasing a list element invalidates any iterator that refers to that element, so attempting to carry on iterating afterwards will cause undefined behaviour. If there will only be one element to remove, then return from the function straight after the call to erase; otherwise, the loop needs to be structured something like:

for (auto it = list.begin(); it != list.end(); /* don't increment here */) {
    if (it->ID == id) {
        it = list.erase(it);
    } else {
        ++it;
    }
}

Thank you so much! I totally forgot to pass by reference – Kyle Preiksa Oct 01 '12 at 02:57 — Kyle Preiksa, Oct 01 '12 at 02:57

score 6 · Answer 2 · answered Oct 01 '12 at 02:27

Calling erase() when an iterator is iterating over the list invalidates the iterator. Add the elements to erase to a second list then remove them afterwards.

Also note that you are passing the list by value rather than using a reference or a pointer. Did you mean to use list<Process>& listToDeleteFrom or list<Process>* listToDeleteFrom?

chris · Answer 3 · 2012-10-01T02:42:01.127

The reason you see no changes reflected is that your list is not being passed by reference, so you are only removing elements from a copy of the list.

Change it to this:

void FileReader::DeleteProcess(int id, list<Process> &listToDeleteFrom) //note &

This will keep the same syntax in the function and modify the original.

However, the way you're deleting the elements is a bit sub-optimal. If you have C++11, the following will remove your invalidation problem, and is more idiomatic, using an existing algorithm designed for the job:

listToDeleteFrom.erase ( //erase matching elements returned from remove_if
    std::remove_if( 
        std::begin(listToDeleteFrom), 
        std::end(listToDeleteFrom), 
        [](const Process &p) { //lambda that matches based on id
            return p->ID == id;
        }
    ),
    std::end(listToDeleteFrom) //to the end of the list
);

Note the keeping of std::list<>::erase in there to actually erase the elements that match. This is known as the erase-remove idiom.

std::list::erase not working

3 Answers3

Linked

Related