CODEWITHSUNDEEP
c++vectorstl
Czoka
Czoka

Reputation: 118

C++ STL vector erase

The problem is that when I run the function it crashes at the erase part and I cant figure out why.

void Grupa::del() {
    int size = studenti.size();
    for (int i=0; i<size; i++) {
        if (studenti[i].materia1<5 && studenti[i].materia2<5 && studenti[i].materia3<5) {
        studenti.erase(studenti.begin()+i);
        }
    }
}

Upvotes: 2

Views: 242

Answers (3)

Mike Seymour
Mike Seymour

Reputation: 254731

When you erase an element, the vector gets smaller; but you're still using the original size, and falling off the end. Also, you don't want to increment i after erasing, otherwise you'll skip the element after the erased one. So you want something like:

for (size_t i = 0; 
     i != studenti.size(); // don't hoist out of the loop
     /* don't increment here */) 
{
    if (...) {
        studenti.erase(studenti.begin()+i);
    } else {
        ++i;
    }
}

Or see other answers for the "erase-remove" idiom, which is a nice, and perhaps more efficient, way of avoiding this kind of error-prone logic.

Upvotes: 12

jmihalicza
jmihalicza

Reputation: 2089

i goes beyond the size of the vector. Due to the erase calls the size of the vector gets smaller, but you go until the saved size, which is greater than the actual if there were erased items.

This is why erase returns an iterator, it is a valid iterator to the element next to the erased one:

for (auto it = studenti.begin(); it != studenti.end();) {
    if (it->materia1<5 && it->materia2<5 && it->materia3<5)
        it = studenti.erase(it);
    else
        ++it;
}

Upvotes: 0

Billy ONeal
Billy ONeal

Reputation: 106609

It looks like you should be using an STL algorithm, std::remove_if, instead of this, which conveniently avoids the problems other answerers have pointed out already. Consider this instead:

studenti.erase(std::remove_if(studenti.cbegin(), studenti.cend(), [](Student const& currentStudent) {
    return currentStudent.materia1<5 && currentStudent.materia2<5 && currentStudent.materia3<5;
}), studenti.cend());

Note that this has the advantage over your solution in that it takes linear time relative to the number of elements in the vector, while the for/erase solution takes quadratic time.

Upvotes: 6

Related Questions