Use Destructor to Delete Heap-allocated Objects in Member Vector

Question

I have a recursive problem I'm trying to solve. If a given action is possible, there may exist additional child actions, and so forth. My solution is a class like this:

// MyObj.h
#include <vector>
class MyObj
{
    private:
        std::vector<MyObj*> _children;
    public:
        void addChild(MyObj* m);
}

// MyObj.cpp
#include "MyObj.h"
void MyObj::addChild(MyObj* m)
{
    MyObj::_children.push_back(m);
}

I'm using the class like this:

MyObj m;
MyObj *child = new MyObj();
m.addChild(child);

My understanding is that since I allocated child on the heap, I need to destroy it later. If the code that creates that object doesn't maintain that reference, it's going to be up to the parent object to destroy it. Is it appropriate to define a destructor like this:

MyObj::~MyObj()
{
    for (std::size_t i = 0; i < MyObj::_children.size(); i++)
    {
        delete MyObj::_children[i];
    }
}

Am I on the right track with this, or is my approach flawed?

PS: I apologize if this is a direct duplicate, as I know there are lots of questions already dealing with destructors; I read a bunch but still didn't feel confident. I am pretty inexperienced with C++ and figured a direct question would be most helpful to me.

You should use smart pointers such as [`std::unique_ptr`](http://en.cppreference.com/w/cpp/memory/unique_ptr) or [`std::shared_ptr`](http://en.cppreference.com/w/cpp/memory/shared_ptr) to handle object ownership instead of raw pointers. As a side effect, your question would be moot. — François Andrieux, May 04 '17 at 18:54
You have to make the decision on whether your class owns its children. If it does then you will be required to do your code above. — adamfowlerphoto, May 04 '17 at 19:05
I would like to point out smart pointers aren't always going to save the day. They bring along their own issues. Check out http://stackoverflow.com/questions/1905787/pros-and-cons-of-smart-pointers before deciding to use them throughout your project. — adamfowlerphoto, May 04 '17 at 19:22
Thanks @Spads. I don't think it's a huge deal for the class to own the children, as each parent object will be declared on the stack and eventually have its destructor called. I will look into smart pointers though; they're a feature I never learned about back when I was taught C++. Thanks everyone for the responses. — Justin K, May 05 '17 at 12:47

score 1 · Accepted Answer · edited May 23 '17 at 12:34

You shouldn't use new unless absolutely necessary. Once you do so it is your responsibility to deallocate the memory you dynamically allocated. In your code above:

MyObj m;
MyObj *child = new MyObj();
m.addChild(child);

Once the function goes out of scope, m and child will have their destructor functions called upon since they are not dynamically allocated, thus destroying them both.

However, the contents pointed to by the child pointer are not destroyed in a similar manner as m and child since it was dynamically allocated in the free store via new. In that case you would need to call delete for every object you placed on the free store, like you are doing above.

This is why in the comments people are suggesting that you use smart pointers, since they follow the RAII paradigm, in which case the object(s) will automatically be deallocated once they fall out of scope.

Use Destructor to Delete Heap-allocated Objects in Member Vector

1 Answers1