Reputation: 1276
when I'm running the following code in enter link description here and when the erase is preform the iterator is been promoted (the iterator is advance to the next element ) and on my real code it not (i know it not suppose to promote my iterator and when I'm using erase the iterator become invalid) but why on the simulator it happen?
*EDIT this is the code:
// Example program
#include <iostream>
#include <string>
#include <vector>
struct A {
A(int _a):a{_a}{}
int a;
};
struct B {
B()
{
vec.push_back(new A(1));
vec.push_back(new A(2));
}
std::vector<A*> vec;
void clean()
{
for(auto e = vec.begin(); e != vec.end(); )
{
auto it = *e;
std::cout << it->a << std::endl;
vec.erase(e);
}
std::cout << vec.size() << std::endl;
}
};
int main()
{
B b;
b.clean();
return 0;
}
Upvotes: 1
Views: 131
Reputation: 17454
So you're asking why an invalidated, erase
'd iterator appears to be incremented to the next item in the range.
This is a textbook case of undefined behaviour.
The practical results depend on a wealth of things. The container implementation, the iterator implementation, compiler optimisations, the time of day, your star sign…
It appears that in one case you are observing a basic, pointer-based iterator "just so happening" to physically point to a buffer of data that is still valid, after all the container's elements have been shuffled down to service your erase
call.
But trying to read that is not valid. Iterators aren't just pointers, and pointers aren't just numbers pointing to memory locations. They have a conceptual purpose and both the compiler and the standard library implementations use this fact to perform all sorts of magic under the bonnet. You are just "getting lucky" in that case, as is shown when you tried to do the same on another computer (in your "real code") and got a different result.
Simply don't do this. Use the return value of erase
to proceed.
Upvotes: 1
Reputation: 12047
vector
's erase
function returns an iterator that references the first element after the erased elements. Use the return value by changing
vec.erase(e);
to
e = vec.erase(e);
You also have a memory leak, since the vector is the only way to access the instances of A
, so you should delete them when you remove them from the vector:
delete it;
e = vec.erase(e);
However, even better would be to not allocate the A
s with new
, but to let the vector manage the lifetime:
vec.push_back(A(1));
the rest of the code would have to be changed too, and you would not need to use delete
.
Upvotes: 6
Reputation: 11350
Modifying a vector while iterating over it is a big no-no in most cases. The reference states that erase()
Invalidates iterators and references at or after the point of the erase, including the end() iterator.
Which means you encounter undefined behaviour here. Not to mention that it's horribly inefficient to erase the first elementt of a vector until it's empty.
Be aware that erasing an element of the vector only removes the A*
from the vector, it does not free the memory it's pointing to, so you are leaking memory.
Also note, that you never increment e
, so that loop doesn't do anything, except dereferencing invalidated iterators.
A better implementaion might be this:
void clean()
{
for(auto item : vec) // automatic range based loop
{
std::cout << item->a << std::endl; //print some value
delete item; // delete the pointer, thus freeing the memory and avoir leaking
}
vec.clear(); // erase all elements at once
std::cout << vec.size() << std::endl;
}
Upvotes: 1
Reputation: 4713
vector
's iterators are invalidated upon size change of vector. So your code is technically UB as erase
changes size of the vector
.
Frequently in implementations iterators just point towards the same piece of data in memory while vector's data is reallocated or modified upon size change.
Upvotes: 1