Best way to delete memory for objects from vector

Question

I have vector which contains the objects allocated on heap. I want to delete some of the element from the vector. Which is the best way to remove element from vector and delete it.

The one way is tried in the below code:

class MyObj
{
    bool rem;
public:
    MyObj(bool r) : rem(r) { cout << "MyObj" << endl; }
    ~MyObj() { cout << "~MyObj" << endl; }

    bool shouldRemove() const noexcept { return rem; }
};


int main()
{
    vector<MyObj*> objs;
    objs.push_back(new MyObj(false));
    objs.push_back(new MyObj(true));
    objs.push_back(new MyObj(false));
    objs.push_back(new MyObj(true));

    auto itr =  objs.begin();
    while (itr != objs.end())
    {
        if ((*itr)->shouldRemove())
        {
            delete *itr;
            *itr = nullptr;

            itr = objs.erase(itr);
        }
        else
            ++itr;
    }

    // size will be two
    cout << "objs.size() :" << objs.size() << endl;

    return 0;
}

Answer 1

Your loop is fine, as far as removing and delete'ing objects (the nullptr assignment is unnecessary). But the rest of your code is prone to memory leaks. If push_back() throws, you leak the object you just new'ed. And you are not delete'ing the objects that are still in the vector after your loop ends.

Which is the best way to remove element from vector and delete it

The best option is to not use raw pointers at all. Store actual object instances directly in the vector, and let the vector destruct the instances for you when you remove them, and when the vector itself is destructed when it goes out of scope:

int main() {
    std::vector<MyObj> objs;

    objs.emplace_back(false);
    objs.emplace_back(true);
    objs.emplace_back(false);
    objs.emplace_back(true);

    auto itr = objs.begin();
    while (itr != objs.end()) {
        if (itr->shouldRemove())
            itr = objs.erase(itr);
        else
            ++itr;
    }

    /* alternatively:
    objs.erase(
        std::remove_if(objs.begin(), objs.end(),
            [](auto &o){ return o.shouldRemove(); }),   
        objs.end()
    );
    */

    // size will be two
    std::cout << "objs.size() :" << objs.size() << std::endl;

    return 0;
}

Otherwise, if you need to store pointers to dynamically allocated objects, at least use smart pointers to manage them:

int main() {
    std::vector<std::unique_ptr<MyObj>> objs;

    objs.push_back(std::unique_ptr<MyObj>(new MyObj(false)));
    objs.push_back(std::unique_ptr<MyObj>(new MyObj(true)));
    objs.push_back(std::unique_ptr<MyObj>(new MyObj(false)));
    objs.push_back(std::unique_ptr<MyObj>(new MyObj(true)));

    /* alternatively, if you are using C++14 or later
    objs.push_back(std::make_unique<MyObj>(false));
    objs.push_back(std::make_unique_ptr<MyObj>(true));
    objs.push_back(std::make_unique<MyObj>(false));
    objs.push_back(std::make_unique<MyObj>(true));
    */

    auto itr = objs.begin();
    while (itr != objs.end()) {
        if ((*itr)->shouldRemove())
            itr = objs.erase(itr);
        else
            ++itr;
    }

    /* alternatively:
    objs.erase(
        std::remove_if(objs.begin(), objs.end(),
            [](auto &o){ return o->shouldRemove(); }),  
        objs.end()
    );
    */

    // size will be two
    std::cout << "objs.size() :" << objs.size() << std::endl;

    return 0;
}