I have a vector of IInventory*, and I am looping through the list using C++11 range for, to do stuff with each one.
After doing some stuff with one, I may want to remove it from the list and delete the object. I know I can call delete
on the pointer any time to clean it up, but what is the proper way to remove it from the vector, while in the range for
loop? And if I remove it from the list will my loop be invalidated?
std::vector<IInventory*> inv;
inv.push_back(new Foo());
inv.push_back(new Bar());
for (IInventory* index : inv)
{
// Do some stuff
// OK, I decided I need to remove this object from 'inv'...
}
std::remove_if
with a predicate that "does stuff" and then returns true if you want the element removed.
i--
after delete. Or iterate backwards with integer indices.
std::list
below
No, you can't. Range-based for
is for when you need to access each element of a container once.
You should use the normal for
loop or one of its cousins if you need to modify the container as you go along, access an element more than once, or otherwise iterate in a non-linear fashion through the container.
For example:
auto i = std::begin(inv);
while (i != std::end(inv)) {
// Do some stuff
if (blah)
i = inv.erase(i);
else
++i;
}
Every time an element is removed from the vector, you must assume the iterators at or after the erased element are no longer valid, because each of the elements succeeding the erased element are moved.
A range-based for-loop is just syntactic sugar for "normal" loop using iterators, so the above applies.
That being said, you could simply:
inv.erase(
std::remove_if(
inv.begin(),
inv.end(),
[](IInventory* element) -> bool {
// Do "some stuff", then return true if element should be removed.
return true;
}
),
inv.end()
);
vector
will never reallocate due to a call to erase
. The reason the iterators are invalidated is because each of the elements succeeding the erased element are moved.
[&]
would be appropriate, to allow him to "do some stuff" with local variables.
remove_if
with .erase
, otherwise nothing happens.
std::remove_if
is O(n).
You ideally shouldn't modify the vector while iterating over it. Use the erase-remove idiom. If you do, you're likely to encounter a few issues. Since in a vector
an erase
invalidates all iterators beginning with the element being erased upto the end()
you will need to make sure that your iterators remain valid by using:
for (MyVector::iterator b = v.begin(); b != v.end();) {
if (foo) {
b = v.erase( b ); // reseat iterator to a valid value post-erase
else {
++b;
}
}
Note, that you need the b != v.end()
test as-is. If you try to optimize it as follows:
for (MyVector::iterator b = v.begin(), e = v.end(); b != e;)
you will run into UB since your e
is invalidated after the first erase
call.
std::remove
, and it's O(N^2) not O(N).
Is it a strict requirement to remove elements while in that loop? Otherwise you could set the pointers you want to delete to NULL and make another pass over the vector to remove all NULL pointers.
std::vector<IInventory*> inv;
inv.push_back( new Foo() );
inv.push_back( new Bar() );
for ( IInventory* &index : inv )
{
// do some stuff
// ok I decided I need to remove this object from inv...?
if (do_delete_index)
{
delete index;
index = NULL;
}
}
std::remove(inv.begin(), inv.end(), NULL);
sorry for necroposting and also sorry if my c++ expertise gets in the way of my answer, but if you trying to iterate through each item and make possible changes (like erasing an index), try using a backwords for loop.
for(int x=vector.getsize(); x>0; x--){
//do stuff
//erase index x
}
when erasing index x, the next loop will be for the item "in front of" the last iteration. i really hope this helped someone
OK, I'm late, but anyway: Sorry, not correct what I read so far - it is possible, you just need two iterators:
std::vector<IInventory*>::iterator current = inv.begin();
for (IInventory* index : inv)
{
if(/* ... */)
{
delete index;
}
else
{
*current++ = index;
}
}
inv.erase(current, inv.end());
Just modifying the value an iterator points to does not invalidate any other iterator, so we can do this without having to worry. Actually, std::remove_if
(gcc implementation at least) does something very similar (using a classic loop...), just does not delete anything and does not erase.
Be aware, however, that this is not thread safe(!) - however, this applies, too, for some of the other solutions above...
erase
(provided you delete more than one single element, of course)?
I will show with example, the below example remove odd elements from vector:
void test_del_vector(){
std::vector<int> vecInt{0, 1, 2, 3, 4, 5};
//method 1
for(auto it = vecInt.begin();it != vecInt.end();){
if(*it % 2){// remove all the odds
it = vecInt.erase(it);
} else{
++it;
}
}
// output all the remaining elements
for(auto const& it:vecInt)std::cout<<it;
std::cout<<std::endl;
// recreate vecInt, and use method 2
vecInt = {0, 1, 2, 3, 4, 5};
//method 2
for(auto it=std::begin(vecInt);it!=std::end(vecInt);){
if (*it % 2){
it = vecInt.erase(it);
}else{
++it;
}
}
// output all the remaining elements
for(auto const& it:vecInt)std::cout<<it;
std::cout<<std::endl;
// recreate vecInt, and use method 3
vecInt = {0, 1, 2, 3, 4, 5};
//method 3
vecInt.erase(std::remove_if(vecInt.begin(), vecInt.end(),
[](const int a){return a % 2;}),
vecInt.end());
// output all the remaining elements
for(auto const& it:vecInt)std::cout<<it;
std::cout<<std::endl;
}
output aw below:
024
024
024
Keep in mind, the method erase
will return the next iterator of the passed iterator.
From here , we can use a more generate method:
template<class Container, class F>
void erase_where(Container& c, F&& f)
{
c.erase(std::remove_if(c.begin(), c.end(),std::forward<F>(f)),
c.end());
}
void test_del_vector(){
std::vector<int> vecInt{0, 1, 2, 3, 4, 5};
//method 4
auto is_odd = [](int x){return x % 2;};
erase_where(vecInt, is_odd);
// output all the remaining elements
for(auto const& it:vecInt)std::cout<<it;
std::cout<<std::endl;
}
See here to see how to use std::remove_if
. https://en.cppreference.com/w/cpp/algorithm/remove
In opposition to this threads title, I'd use two passes:
#include <algorithm>
#include <vector>
std::vector<IInventory*> inv;
inv.push_back(new Foo());
inv.push_back(new Bar());
std::vector<IInventory*> toDelete;
for (IInventory* index : inv)
{
// Do some stuff
if (deleteConditionTrue)
{
toDelete.push_back(index);
}
}
for (IInventory* index : toDelete)
{
inv.erase(std::remove(inv.begin(), inv.end(), index), inv.end());
}
A much more elegant solution would be to switch to std::list
(assuming you don't need fast random access).
list<Widget*> widgets ; // create and use this..
You can then delete with .remove_if
and a C++ functor in one line:
widgets.remove_if( []( Widget*w ){ return w->isExpired() ; } ) ;
So here I'm just writing a functor that accepts one argument (the Widget*
). The return value is the condition on which to remove a Widget*
from the list.
I find this syntax palatable. I don't think I would ever use remove_if
for std::vectors -- there is so much inv.begin()
and inv.end()
noise there you're probably better off using an integer-index-based delete or just a plain old regular iterator-based delete (as shown below). But you should not really be removing from the middle of a std::vector
very much anyway, so switching to a list
for this case of frequent middle of list deletion is advised.
Note however I did not get a chance to call delete
on the Widget*
's that were removed. To do that, it would look like this:
widgets.remove_if( []( Widget*w ){
bool exp = w->isExpired() ;
if( exp ) delete w ; // delete the widget if it was expired
return exp ; // remove from widgets list if it was expired
} ) ;
You could also use a regular iterator-based loop like so:
// NO INCREMENT v
for( list<Widget*>::iterator iter = widgets.begin() ; iter != widgets.end() ; )
{
if( (*iter)->isExpired() )
{
delete( *iter ) ;
iter = widgets.erase( iter ) ; // _advances_ iter, so this loop is not infinite
}
else
++iter ;
}
If you don't like the length of for( list<Widget*>::iterator iter = widgets.begin() ; ...
, you can use
for( auto iter = widgets.begin() ; ...
remove_if
on a std::vector
actually work, and how it keeps the complexity to O(N).
std::vector
is always going to slide each element after the one you removed up one, making a std::list
a much better choice.
remove_if
will slide each element up by the number of spaces freed. By the time you account for cache usage, remove_if
on a std::vector
likely outperforms removal from a std::list
. And preserves O(1)
random access.
I think I would do the following...
for (auto itr = inv.begin(); itr != inv.end();)
{
// Do some stuff
if (OK, I decided I need to remove this object from 'inv')
itr = inv.erase(itr);
else
++itr;
}
you can't delete the iterator during the loop iteration because iterator count get mismatch and after some iteration you would have invalid iterator.
Solution: 1) take the copy of original vector 2) iterate the iterator using this copy 2) do some stuff and delete it from original vector.
std::vector<IInventory*> inv;
inv.push_back(new Foo());
inv.push_back(new Bar());
std::vector<IInventory*> copyinv = inv;
iteratorCout = 0;
for (IInventory* index : copyinv)
{
// Do some stuff
// OK, I decided I need to remove this object from 'inv'...
inv.erase(inv.begin() + iteratorCout);
iteratorCout++;
}
Erasing element one-by-one easily leads to N^2 performance. Better to mark elements that should be erased and erase them at once after the loop. If I may presume nullptr in not valid element in your vector, then
std::vector<IInventory*> inv;
// ... push some elements to inv
for (IInventory*& index : inv)
{
// Do some stuff
// OK, I decided I need to remove this object from 'inv'...
{
delete index;
index =nullptr;
}
}
inv.erase( std::remove( begin( inv ), end( inv ), nullptr ), end( inv ) );
should work.
In case your "Do some stuff" is not changing elements of the vector and only used to make decision to remove or keep the element, you can convert it to lambda (as was suggested in somebody's earlier post) and use
inv.erase( std::remove_if( begin( inv ), end( inv ), []( Inventory* i )
{
// DO some stuff
return OK, I decided I need to remove this object from 'inv'...
} ), end( inv ) );
Success story sharing
true
, AFAIU, and it seems better this way to not mix iteration logic in with the predicate.remove_if
is better.erase
returns a new valid iterator. it might not be efficient, but it's guaranteed to work.