I have a vector

std::vector<Object*> objects;

And a method that removes an object if it's found:

void Remove(Object *o)
{
    objects.erase(
        std::remove_if(
            objects.begin(), objects.end(),
            [&o](Object *_object) {
                if (o == _object)
                {
                    delete _object;
                    return true;
                }
                return false;
            }
        ),
        objects.end()
    );
}

Is this safe? Should I not call delete? But then would erase call delete for me? I'm a bit confused. Would this invalidate the iterator or leak memory?

CodePudding user response：

Would this invalidate the iterator or leak memory?

Memory doesn't get leaked by individual bits of code; it gets leaked by complete programs. Someone has to be responsible for deallocation, but just because it doesn't happen here, doesn't mean it won't happen at all. Not calling delete here would also not "leak memory"; what leaks memory is the entire program. Code like this cannot stand on its own.

It's difficult to figure out where the responsibility lies, except by following established patterns. That's why tools like std::unique_ptr<T>, std::shared_ptr<T> and std::weak_ptr<T> exist. Please use them.

The iterator will not be invalidated. The iterator is iterating over elements of the container, which are the pointers themselves. delete does not affect the pointers it's used with. delete affects the pointed-at memory.

Would erase call delete for me?

No.

Is this safe?

Code like this risks a double deallocation (undefined behaviour) if any other code could possibly attempt to delete any of the same pointed-at elements from the container.

Again, this is not a local risk, it is a whole-program risk.

Again, figuring out memory management across a whole-program context is difficult in general. Please use standard library tools like std::unique_ptr<T>, std::shared_ptr<T> and std::weak_ptr<T>, as appropriate to the situation, in appropriate ways. A proper tutorial on memory management in C is beyond the scope of a Stack Overflow question.

CodePudding user response：

From std::remove_if

UnaryPredicate must meet the requirements of Predicate.

and from there:

The function object pred shall not apply any non-constant function through the dereferenced iterator. This function object may be a pointer to function or an object of a type with an appropriate function call operator.

(It should be possible to call the predicate twice on same element).

Your predicate doesn't respect that requirement, so code is unsafe.

Keeping your logic, you might use std::partition:

void Remove(/*const*/ Object *o)
{
    auto it = std::partition(objects.begin(), objects.end(),
                             [&o](const Object* object) { return o != object; }
                            );
    for (auto it2 = it; it2 != objects.end();   it2) { delete *it2; }
    objects.erase(it, objects.end());
}

but using smart pointers (as std::unique_ptr) would simplify the code

void Remove(/*const*/ Object *o)
{
    objects.erase(
        std::remove_if(
            objects.begin(), objects.end(),
            [&o](const std::unique_ptr<Object>& object) {
                return o != object.get();
            }
        ),
        objects.end()
    );
}