Inserting element into vector during iteration causes crash

Steve Hatcher · 2016-09-21T18:57:12

Hi All, I have a situation in my code where by during iteration of my vector of pointers, I am modifying the original container. I have done some research and learned this causes a crash (I Think)... The problem I am facing is illustrated in my test program as follows: #include <vector> #include <iostream> #include <memory> using namespace std; struct Test1 { void update(){ cout << "update from Test1" << endl; } }; struct Test { vector<shared_ptr<Test1>> m_test; void pushOn(std::shared_ptr<Test1> &test1) { m_test.push_back(test1); } void update() { for (vector<shared_ptr<Test1>>::reverse_iterator i = m_test.rbegin(); i != m_test.rend(); ++i) { auto newPtr = make_shared<Test1>(); (*i)->update(); pushOn(newPtr); cout << "if you read this it passed" << endl; } } }; int main() { auto TestClass = make_shared<Test>(); auto aaa = make_shared<Test1>(); TestClass->pushOn(aaa); TestClass->update(); getchar(); return 0; } If you run the program with pushOn(newPtr); commended out, then it works, otherwise it crashes. I have some questions. 1. May I have some advice on how to fix this? 2. If I am encountering this, does that mean a fundamentally flawed program design? Thanks

For Beginners

Started by SteveHatcher September 20, 2016 10:17 AM

12 comments, last by Khatharr 7 years, 7 months ago

SteveHatcher

1,237

Author

September 21, 2016 02:21 AM

It does do exactly that. But you also call resize(10), which changes the vector to have 10 elements, and since you only had 10 capacity, it's full up again.

Ahh okay, but when I remove the resize(10) I still get the crash, so I am a bit confused now...

I thought this sort of thing would be quite common in C++ so am surprised at this behavior!

Documentation of std::vector will point out that element insertion invalidates iterators.

It looks like this vector is a primary owner for these objects. If you can ensure that the vector outlives anything that may refer to its contents then you could just use std::vector<Test1> and then take references to elements or (preferably) simply refer to them directly as needed. If you're thinking about polymorphism then that won't work though.

The actual situation in my game needs polymorphism, thanks though!

Thanks everyone for your input I am learning a lot!

vstrakh

2,459

September 21, 2016 07:47 AM

Ahh okay, but when I remove the resize(10) I still get the crash

Why do you reserve space for exactly 10 elements?

You double your m_test vector content with each update loop.

If you reserved space for 10 elements and there's already 5 elements in m_test, then you can run update just once. Next iteration will fail.

And if there's already more than 5 existing elements in m_test, then update loop will fail, because doubled vector content won't fit in reserved 10 elements.

And even if you will reserve memory before each update to allow your loop run many times, you'll consume entire ram in ~10-20 updates, depending on available ram size and its fragmentation.

Kylotan

10,510

September 21, 2016 08:45 AM

Ahh okay, but when I remove the resize(10) I still get the crash, so I am a bit confused now...

I don't think you'll see it on the first push_back, but you will probably see it on the eleventh. If that's not what you experience, post your code.

Beyond that, if you're still stuck for how to proceed in the general case, there are a few suggestions above:
a) postpone additions until the end of the loop (e.g. by adding to a separate temporary vector),
b) switch to std::deque which allows push_back during iteration,
c) restart iteration from the beginning any time you push_back
d) call reserve() once (and only once) with the maximum capacity, and take steps to ensure you never exceed that