Deleting last element in a std::vector of std::shared_ptr while iterating over it causes segmentation fault

Question

please try to compile and run the following code. When iterating over a vector of shared pointers, I have to delete last element, this will result in a segmentation fault, but I can't understand why the for iteration doesn't break when el_it reaches v.end(), and I have to do it manually (commented code).

#include <vector>
using std::vector;

#include <memory>
using std::shared_ptr;

#include <algorithm>
using std::remove;

class A {
public:
    A(int age) { age_ = age; }
    int age_ = 0;
    int alive_ = 1;
};

int main() {

    shared_ptr<A> a0(new A(0));
    shared_ptr<A> a1(new A(1));
    shared_ptr<A> a2(new A(2));
    vector< shared_ptr <A> > v;
    v.push_back(a0);
    v.insert(v.end(), a1);
    v.insert(v.end(), a2);
    for (auto el_it = v.begin(); el_it != v.end(); ++ el_it) {
        auto el = *el_it;
        if (el->age_ == 2) {
            v.erase(el_it);
        }
        /*
        if (el_it == v.end()) // Why is this required ??
            break;
        */

    }
    return 0;

}

See [Erase-remove_idiom](https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom) — Jarod42, May 03 '19 at 09:46
Related and possibly duplicate of [Iterator invalidation rules](https://stackoverflow.com/questions/6438086/iterator-invalidation-rules) — P.W, May 03 '19 at 09:46
*"`if (el_it == v.end()) // Why is this required ??`"*, in fact no, there is still UB. — Jarod42, May 03 '19 at 09:48
@LightnessRacesinOrbit: :-). There haven't been any recent additions to the C++faq tag recently. Any contributions on the way? — P.W, May 03 '19 at 09:52

The Quantum Physicist · Accepted Answer · 2019-05-03T09:56:20.183

1

Change:

v.erase(el_it);

To

el_it = v.erase(el_it); // don't do this yet, read below

Erasing invalidates iterators. The return value of erase is the valid iterator to the next element. So with what I wrote above you're skipping one element. I'd recommend this:

for (auto el_it = v.begin(); el_it != v.end();) {
    auto el = *el_it;
    if (el->age_ == 2) {
        el_it = v.erase(el_it);
    } else {
        ++ el_it;
    }
}

edited May 03 '19 at 09:56

answered May 03 '19 at 09:44

The Quantum Physicist

24,987
19
103
189

2

`++ el_it` should be removed too in that case – CinCout May 03 '19 at 09:46
No point doing it now, the question is already dupe-hammered – CinCout May 03 '19 at 09:48
You forgot to incorporate the `el_it = v.erase(el_it);` into the code. – nwp May 03 '19 at 09:54
@nwp Done. Thanks. – The Quantum Physicist May 03 '19 at 09:56
@DomenicoDiIorio The first one is not valid in your case. You have to update the iterator after erasing an element. It was a typo on my part. – The Quantum Physicist May 03 '19 at 09:57

Remy Lebeau · Answer 2 · 2019-05-03T09:59:26.970

Calling std::vector::erase() invalidates your el_it iterator. You are not taking that into account. erase() returns a new iterator to the element that follows the one you have removed. You need to use that new iterator to continue the loop.

Try this instead :

for (auto el_it = v.begin(); el_it != v.end();) {
    auto el = *el_it;
    if (el->age_ == 2) {
        el_it = v.erase(el_it);
    } else {
        ++el_it;
    }
}

Alternatively, a better option is to use the erase-remove idiom via the std::remove_if() algorithm, instead of looping manual at all:

v.erase(
    std::remove_if(v.begin, v.end(),
        [](auto el){ return (el->age_ == 2); } 
    ),
    v.end()
);

C++20 adds std::erase_if() to make it easier:

std::erase_if(v,
    [](auto el){ return (el->age_ == 2); } 
);

Deleting last element in a std::vector of std::shared_ptr while iterating over it causes segmentation fault

2 Answers2