1

I need to loop over some objects of class T.

They are stored in an std::set<std::unique_ptr<T>> tees.

The main purpose of the loop's body is to use the objects, but by doing that I will also find out when some of the objects are no longer needed and can be deleted.

I am using a range-based for loop for iterating over the unique_ptrs:

for (std::unique_ptr<T> & tee : tees)

I know I cannot call tees.erase(tee) inside the loop (UB). Therefore I should collect the unique_ptrs that need to be deleted in a helper collection. Problem: The pointers are unique, therefore I cannot copy them into the helper collection.

I could collect the raw pointers in a std::set<T*>, but how would I use these after the loop to delete the matching unique_ptrs from the tees collection? Also, collecting the raw pointers again somehow feels wrong when I made the effort to use smart pointers in this problem.

I could switch to shared_ptr, but the pointers would only ever be shared for the purpose of deleting the objects. Does not feel right.

I could switch from range-based for to something else, like handling the iterators myself, and get the next iterator before deleting the entry. But going back to pre-C++11 techniques also does not feel right.

I could switch to std::remove_if. (EDIT: I can't, actually. Explained in comments below this question and below the accepted answer.) The loop's body would move into the unary_predicate lambda. But the main purpose of the loop is not to determine whether the objects should be deleted, but to make use of them, altering them.

The way of least resistance seems to be to go back to iterator-handling, that way I do not even need a helper collection. But I wonder if you can help me with a C++11-ish (or 14,17) solution?

T. Herzke
  • 627
  • 4
  • 12
  • 4
    Personally I would just use a regular iterator loop and erase the pointer once you are done with it. Just because the technique is not new does not mean it is bad. – NathanOliver Aug 28 '18 at 16:43
  • 1
    "But going back to pre-C++11 techniques also does not feel right." I am afraid you got wrong filling. For range loop simplifies certain cases. But it does not mean you have to use it when it makes things more difficult. – Slava Aug 28 '18 at 16:45
  • 1
    And you cannot use `std::remove_if` with `std::set` – Slava Aug 28 '18 at 16:47
  • @Galik no you cannot, that will invalidare `std::set` invariant – Slava Aug 28 '18 at 17:02

3 Answers3

4

I don't think you are going to find anything easier than

for(auto it = container.begin(), it != container.end();)
{
    //use *it here
    if(needs_to_be_erased)
        it = container.erase(it);
    else
        ++it;
}

since std::set does not provide mutable access to its elements any sort of transform or remove will not work. You would have to build a container of iterators and then after you process the set go through that container of iterators calling erase for each one.

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • If standard library would provide `std::erase_if` that would help, but unfortunately without ranges process of creating new algos is unnecessary difficult. – Slava Aug 28 '18 at 16:53
  • @Slava It would be really nice if all the containers had an `erase_if`. It would make generic programming easier. – NathanOliver Aug 28 '18 at 16:54
  • I do not follow, why containers have to have it? `std::remove_if` is not a method of `std::vector` is it? – Slava Aug 28 '18 at 16:56
  • @Slava remove doesn't actually remove anything, it just shuffles the stuff to the end. If we had a `erase_if` we wouldn't need a remove call followed by an erase call. – NathanOliver Aug 28 '18 at 16:57
  • Ah yes I forgot, you cannot modify container by iterators. Yet another problem of that design. – Slava Aug 28 '18 at 16:58
  • I don't understand, the example at cppreference.com does exactly this https://en.cppreference.com/w/cpp/container/set/erase Is cppreference wrong in this? I tend to trust it very much. – Michael Surette Aug 28 '18 at 18:09
  • @MichaelSurette You don't understand what? – NathanOliver Aug 28 '18 at 18:10
  • This answer does better iterator handling than anything I could have come up with. I'll take it, thank you. – T. Herzke Aug 28 '18 at 18:14
  • @NathanOliver I don't understand why you cannot modify container by iterators. The answer here does the same as the example at cppreference.com I referenced. – Michael Surette Aug 28 '18 at 18:16
  • @MichaelSurette You can't modify a `std::set` using a call to `transform` or `remove_if`. The only way to do it is to have a loop like I have here and what cppreferece has. If the object were a vector you wouldn't need the loop and could use something like `vector.erase(std::remove_if(vector.begin(), vector.end(), some_predicate), vector.end());`. Does that make sense? – NathanOliver Aug 28 '18 at 18:20
0

I think you can copy the positions into a new data structure and remove these items in another loop by accessing the new data structure in reverse order.

int counter =0;
vector<int> indices;
for (unique_ptr<T> & tee : tees)
{
    if (bCondition)
        indices.push_back(counter);
    counter++;
}

reverse(indices.begin(), indices.end());
for (int  i : indices)
    tees.erase(tees.begin() + i);
Ahmed Elias
  • 26
  • 3
0

Not exactly a solution but if you have to do this a lot you could make your own algorithm for it. I guess the reason this is not in the standard library is because the algorithm needs to know the container to perform an erase.

So you could do something like this:

template<typename Cont, typename Pred>
void erase_if(Cont& c, decltype(std::begin(c)) b, decltype(std::end(c)) e, Pred p)
{
    while(b != e)
    {
        if(p(*b))
            b = c.erase(b);
        else
            ++b;
    }
}

template<typename Cont, typename Pred>
void erase_if(Cont& c, Pred p)
    { erase_if(c, std::begin(c), std::end(c), p); }

Then call it something like:

erase_if(tees, [](std::unique_ptr<int> const& up){
    // use up here...
    return (*up) & 1; // erase if odd number
});

or

erase_if(tees, std::begin(tees), std::end(tees), [](std::unique_ptr<int> const& up){
    // use up here...
    return (*up) & 1; // erase if odd number
});
Galik
  • 47,303
  • 4
  • 80
  • 117