I was wondering if this is an accaptable practice:
struct Item { };
std::list<std::shared_ptr<Item>> Items;
std::list<std::shared_ptr<Item>> RemovedItems;
void Update()
{
Items.push_back(std::make_shared<Item>()); // sample item
for (auto ItemIterator=Items.begin();ItemIterator!=Items.end();ItemIterator++)
{
if (true) { // a complex condition, (true) is for demo purposes
RemovedItems.push_back(std::move(*ItemIterator)); // move ownership
*ItemIterator=nullptr; // set current item to nullptr
}
// One of the downsides, is that we have to always check if
// the current iterator value is not a nullptr
if (*ItemIterator!=nullptr) {
// A complex loop where Items collection could be modified
}
}
// After the loop is done, we can now safely remove our objects
RemovedItems.clear(); // calls destructors on objects
//finally clear the items that are nullptr
Items.erase( std::remove_if( Items.begin(), Items.end(),
[](const std::shared_ptr<Item>& ItemToCheck){
return ItemToCheck==nullptr;
}), Items.end() );
}
The idea here is that we're marking Items container could be effected by outside sources. When an item is removed from the container, it's simply set to nullptr but moved to RemovedItems before that.
Something like an event might effect the Items and add/remove items, so I had to come up with this solution.
Does this seem like a good idea?
I think you are complicating things too much. If you are a in multi-threaded situation (you didn't mention it in your question), you would certainly need some locks guarding reads from other threads that access your modified lists. Since there are no concurrent data structures in the Standard Library, you would need to add such stuff yourself.
For single-threaded code, you can simply call the std:list member remove_if with your predicate. There is no need to set pointers to null, store them and do multiple passes over your data.
#include <algorithm>
#include <list>
#include <memory>
#include <iostream>
using Item = int;
int main()
{
auto lst = std::list< std::shared_ptr<Item> >
{
std::make_shared<int>(0),
std::make_shared<int>(1),
std::make_shared<int>(2),
std::make_shared<int>(3),
};
// shared_ptrs to even elements
auto x0 = *std::next(begin(lst), 0);
auto x2 = *std::next(begin(lst), 2);
// erase even numbers
lst.remove_if([](std::shared_ptr<int> p){
return *p % 2 == 0;
});
// even numbers have been erased
for (auto it = begin(lst); it != end(lst); ++it)
std::cout << **it << ",";
std::cout << "\n";
// shared pointers to even members are still valid
std::cout << *x0 << "," << *x2;
}
Note that the elements have been really erased from the list, not just put at the end of the list. The latter effect is what the standard algorithm std::remove_if would do, and after which you would have to call the std::list member function erase. This two-step erase-remove idiom looks like this
// move even numbers to the end of the list in an unspecified state
auto res = std::remove_if(begin(lst), end(lst), [](std::shared_ptr<int> p){
return *p % 2 == 0;
});
// erase even numbers
lst.erase(res, end(lst));
However, in both cases, the underlying Item elements have not been deleted, since they each still have a shared pointer associated to them. Only if the refence counts would drop to zero, would those former list elements actually be deleted.