Recursive Destructors in C++

Question

Suppose I have a class similar to the following:

#include <vector>

class element
{
public:
   element();
   ~element();

   virtual void my_func();

private:
   std::vector<element*> _elements;
};

How would I go about implementing the destructor?

I was thinking something like this, but I'm not sure. I am worried about memory leaks since I'm relatively new to C++.

void element::destroy()
{
   for(int i = 0; i < _elements.size(); ++i)
      _elements.at(i)->destroy();

   if(this != NULL) 
      delete this;
}

element::~element()
{
    destroy();
}

Thank you.

PS: Here is a sample main:

int main()
{
   element* el_1 = new element();
   element* el_2 = new element();
   element* el_3 = new element();

   el_1.add_element(el_2);
   el_2.add_element(el_3);

   return 0;
}

Also, what if I do this (aka don't use new):

int main()
{
   element el_1;
   element el_2;
   element el_3;

   el_1.add_element(&el_2);
   el_2.add_element(&el_3);

   return 0;
}

you could call `delete _elements.at(i)` and you should not test that `this` is not null inside a destructor. It cannot be null. — Basile Starynkevitch, Jan 05 '12 at 21:04
Would calling `delete _elements.at(i)` properly dispose of all the sub-elements? In other words, will they be disposed in the proper order (from the bottom up)? Because I think if I delete an `element` that still has stuff in its `_elements`, that will be a leak. — Eric, Jan 05 '12 at 21:06
When you `delete something;` the destructor of `something`'s class is called, and then the pointer is freed. — Basile Starynkevitch, Jan 05 '12 at 21:08
You should avoid raw pointers wherever possible. In this case: make sure that you really need a vector of pointers (usually only required if `element` is an abstract base class or noncopyable), and if so, prefer a `ptr_vector`. — Philipp, Jan 05 '12 at 21:18
@Philipp: In this case, a container of pointers is required: a Standard Library container cannot be instantiated with an incomplete type, so an `element` cannot have a `std::vector` as a member. (Alternatively, one could use a third-party containers library. Boost has a containers library that has less stringent instantiation requirements.) — James McNellis, Jan 05 '12 at 21:40

James McNellis · Accepted Answer · 2012-01-05T21:13:57.597

element::~element()
{
    typedef std::vector<element*>::const_iterator iterator;
    for (iterator it(_elements.begin()); it != _elements.end(); ++it)
        delete *it;
}

delete this in a destructor is always wrong: this is already being destroyed!

In addition, you would need to declare a copy constructor and copy assignment operator (either leaving them undefined, making your class noncopyable, or providing a suitable definition that copies the tree).

Alternatively (and preferably), you should use a container of smart pointers for _elements. E.g.,

std::vector<std::unique_ptr<element>> _elements;

When an element is destroyed, its _elements container will be automatically destroyed. When the container is destroyed, it will destroy each of its elements. A std::unique_ptr owns the object to which it points, and when the std::unique_ptr is destroyed, it will destroy the element to which it points.

By using std::vector<std::unique_ptr<element>> here, you don't need to provide your own destructor, because all of this built-in functionality takes care of the cleanup for you.

If you want to be able to copy an element tree, you'll still need to provide your own copy constructor and copy assignment operator that clone the tree. However, if you don't need the tree to be copyable, you don't need to declare the copy operations like you do if you manage the memory yourself: the std::unique_ptr container is itself not copyable, so its presence as a member variable will suppress the implicitly generated copy operations.

So, in this case, could you think of `delete *it` as being a recursive call to `~element`? — Eric, Jan 05 '12 at 21:10
@Eric: Actually, deleting an object will first call the object's destructor (where it should clean up whatever it is responsible for), and then free the memory the object occupied. — sbi, Jan 05 '12 at 21:18

score 3 · Answer 2 · edited May 23 '17 at 12:25

class element
{
public:
   element();
   ~element();

private:
   std::vector<element*> _elements;
};

Per the Rule of Three, this lacks the definition of copy constructor and assignment operator.

element::~element()
{
    destroy();
}

Calling delete this (which is what destroy() does) from a destructor is always wrong, because the destructor is what is called when the current object is being deleted.

for(int i = 0; i < _elements.size(); ++i)
   _elements.at(i)->destroy();

While this does the job of calling delete on the objects you call destroy() for, this arrangement has the disadvantage that you must not destroy such objects other than through calling destroy(). In particular, this

element el_1;

will be a problem, because e1_1 will be automatically destroyed when it falls out of scope bay calling its destructor. Since this calls destroy(), which invokes delete this on an object not allocated using new, this causes Undefined Behavior. (If you are lucky it blows up into your face right away.)

It would be far better to remove the delete this from the destructor, and only have the destructor delete the objects in the object's vector:

for(std::vector<element*>::size_type i = 0; i < _elements.size(); ++i)
  delete _elements[i];

if(this != NULL) 
   delete this;

Checking a pointer for NULLness is never necessary, because delete is defined to do nothing when the pointer passed to it is NULL.

The final paragraph is misleading; the `if` is never necessary, not the whole snippet (though as a related note of course `delete this` is rarely useful!) — Lightness Races in Orbit, Jan 05 '12 at 21:10

score 2 · Answer 3 · answered Jan 05 '12 at 21:05

The delete this shouldn't be there, as the object is already under destruction by definition.
If you copy an element then all the pointers within its member vector are copied too; then when the original goes out of scope their pointees are destroyed, and the element copy has a vector of dangling pointers. You need a copy ctor and assignment operator.

So, just this basic start has already created two very serious faults. This is a sign that the design is to be avoided. It doesn't appear to be all that clear what the ownership semantics are to the programmer who uses it.

Why is dynamic allocation required here at all? Why not store elements objects themselves?

#include <vector>

class element
{
public:
   void add_element(element const& e) {
      _elements.push_back(e);
   }
private:
   std::vector<element> _elements;
};

This will change your program subtly if you previously had multiple references to the same element going into different other elements, but you'll have to decide for yourself whether that tangled mess of dependencies is something you need.

Thank you for the comments -- the reason I needed to use pointers is because `element` was intended to be a base class with `virtual` functions. This was the only way I could see to ensure the correct form of the virtual function was executed. — Eric, Jan 05 '12 at 21:14
@Eric: OK; that probably makes sense then. Still, consider a wrapped-pointer implementation like `std::shared_ptr`. — Lightness Races in Orbit, Jan 05 '12 at 21:18

score 0 · Answer 4 · answered Jan 05 '12 at 21:07

0

Nope, it doesn't work that way. You never delete this. You just don't. Basically, take out your destroy method, put everything into ~element() except for the delete this part. And instead of calling the destroy method, just delete _elements[i];.

answered Jan 05 '12 at 21:07

Cubic

14,902
5
47
92

`delete this;` is valid and occasionally useful. – James McNellis Jan 05 '12 at 21:15
Yeah, but you still never do it, just like you never do certain other things that you sometimes end up doing anyways because it's convenient. – Cubic Jan 05 '12 at 22:25

Recursive Destructors in C++

4 Answers4