STL deleting from list

Question

Im having trouble with STl. Im trying to iterate through an STL list of student objects. Im trying to delete objects when I find a matching comparison. However I receive an error when doing the comparison. This is what I have done so far:

 string studentName;
                cout<<"Enter name of student to remove";
                cin>>studentName;

                list<Student>::iterator it = studentList.begin();
                while (it != studentList.end()){
                    if(*it== studentName){
                        studentList.erase(it);
                    }
                }

I get the error "Invalid operands to binary expression ('value_type' (aka 'Student') and 'string' (aka 'basic_string, allocator >'))" I'm not quite sure how to solve it. Thank you, any advice is appreciated!

Answer 1

You are comparing an instance of Student with an std::string which I'm assuming doesn't have a defined operator== overloaded function. You can either define this operator or compare studentName with the member string variable in Student which stores the name of the student. You might consider looking at std::remove_if in the algorithms library which you can use to filter out any students which don't have that name.

Answer 2

You are trying to compare Student with string. Such a comparison is not defined by default so either you have to define a proper operator(s) yourself or write something like (*it).getName() == studentName where getName is a member function of Student which returns the name of the student. In addition, your for loop isn't correct. It should be something like this:

for(auto it = studentList.begin(); it != studentList.end();) {
    if((*it).getName() == studentName) {
        it = studentList.erase(it);
    } else {
        ++it;
    }
}

EDIT: If you decide to overload comparison operators then here is a tip on how to do it:

bool operator==(const Student& student, const std::string& name) {
    return student.getName() == name;
}

bool operator==(const std::string& name, const Student& student) {
    return student == name;
}

bool operator!=(const Student& student, const std::string& name) {
    return !(student == name);
}

bool operator!=(const std::string& name, const Student& student) {
    return !(student == name);
}

For the purpose of this question the first of the above four overloads would be enough but usually it's better to define a few versions to avoid any surprises in the future. Also, if the Student class doesn't have any member function like getName (Having such a function is strongly advised unless Student is a simple struct with all data members public.) then you have to change the first overload (The rest of them refer to the first one so they will automatically adjust to the changes.) like this:

bool operator==(const Student& student, const std::string& name) {
    return student.name == name;
}

Furthermore, if the name of the Student is private or protected and there is no way to access it from public context then you also have to add a friend declaration to your Student definition:

class Student {
public:

// Public interface...

private:
    std::string name;

    friend bool operator==(const Student& student, const std::string& name);
};

The position of the friend declaration doesn't matter as long as it's inside the class' definition. Again, you only need to make the first of the overloads privileged because the rest of them just call the first one.
Now the loop can be changed:

for(auto it = studentList.begin(); it != studentList.end();) {
    if(*it == studentName) {
        it = studentList.erase(it);
    } else {
        ++it;
    }
}

Answer 3

When you delete the list segment that the iterator is pointing at, the iterator is no longer valid. This is why erase returns a new iterator to the element following the one that was erased. Also, you might want to increment your iterator at some point in the loop. Try this:

while (it != studentList.end()){
    if(*it == studentName)
        it = studentList.erase(it);
    else
        ++it;
}

Edit: Now that you have posted the error, it's clear that you have another problem. See everyone's answer on how to fix this one.

Answer 4

Your loop is actually equivalent to the following for loop:

for (list<Student>::iterator it = studentList.begin();
     it != studentList.end();
     /* EMPTY */)
{
    if(*it== studentName){
        studentList.erase(it);
    }
}

Notice how that last part of the for loop is empty? That means you you never increment or otherwise modify the variable it in the for statement, and neither does the loop body! That means it will never change and you have an infinite loop.

The simple and obvious way to fix this is to increment it in the loop. Back to your original loop, with the fix added:

list<Student>::iterator it = studentList.begin();
while (it != studentList.end()){
    if(*it== studentName){
        studentList.erase(it);
    }

    ++it;  // Make iterator "point" to the next node
}

However this fix is flawed in another way. This is because when you remove a node, you will skip over the node after the node you remove, so you miss a node. The naive solution is to only increment it if you don't remove a node:

while (it != studentList.end()){
    if(*it== studentName){
        studentList.erase(it);
    } else {
        ++it;  // Make iterator "point" to the next node
    }
}

This solution is flawed in that you will have undefined behavior if you remove a node. This is because then it will not be updated, and the next iteration of the loop you will dereference an iterator to a node that no longer exist. The solution to this problem is to know what the erase function returns, namely an iterator to the following node. That means a working solution would look like

while (it != studentList.end()){
    if(*it== studentName){
        it = studentList.erase(it);  // Make iterator "point" to node after removed node
    } else {
        ++it;  // Make iterator "point" to the next node
    }
}

Answer 5

1. You do not advance the iterator. If you happen to erase the first element, you will crash, otherwise you will get an infinite loop. However, ...
2. There are tons of examples how to iterate lists and erase elements correctly, e.g. here: Erasing while iterating an std::list

Answer 6

You are trying to compare a String and a Student. Also, you are not advancing the iterator, therefore that loop will fail to cease. Try something along these lines:

while (it != studentList.end()) {
    if(it->getName == studentName) {
        it = studentList.erase(it);
    }
    ++it;
}