Is it OK to use "delete this" to delete the current object?

Question

I'm writing a linked list and I want a struct's destructor (a Node struct) to simply delete itself, and not have any side effects. I want my list's destructor to iteratively call the Node destructor on itself (storing the next node temporarily), like this:

//my list class has first and last pointers
//and my nodes each have a pointer to the previous and next
//node
DoublyLinkedList::~DoublyLinkedList
{
    Node *temp = first();

    while (temp->next() != NULL)
    {
        delete temp;
        temp = temp->next();
    }
}

So this would be my Node destructor:

Node::~Node
{
   delete this;
}

Is this acceptable, especially in this context?

Cosmonaut

I'm going to be the last one who will go on a closing rampage but I believe this question has been asked a few times in the past already: google.com/search?q=delete+this+site:stackoverflow.com – Doelling

Answer 1

If the Node destructor is being called, then it's already in the process of being deleted. So a delete doesn't make sense inside your Node destructor.

Also this is wrong:

while (temp->next() != NULL)
{
     delete temp;
     temp = temp->next();
}

Instead you should get temp->next() into a temp variable. Otherwise you are accessing deleted memory.

So more like this:

DoublyLinkedList::~DoublyLinkedList
{
  Node *temp = first();
  while (temp != NULL)
  {
       Node *temp2 = temp->next();
       delete temp;
       temp = temp2;
  }
}

Subaudition

Note: Since you are deleting all the nodes from your DoublyLinkedList destructor, you should not be deleting the Node objects anywhere else. – Subaudition

So do I need a node destructor at all? I just don't want the compiler-supplied dtor messing with my life. – Cosmonaut

You can either put one and leave it blank or you can not put one at all. – Subaudition

@hooked, the compiler-supplied dtor is equivalent to just writing a dtor with an empty body. It will destroy member variables and superclasses no matter what you do - at that point you can't stop it (short of terminating the program first) – Triphylite

So if I were to design a class without heap-allocated memory ala new, would I ever need a constructor, or would it be deleted when it just goes out of scope? – Cosmonaut

If you use new you have to use delete. You delete once per new. Constructors are called whether or not new is called. They are independent concepts. Objects will be freed when new is not used when they go out of scope yes. But for a linked list data structure you would not use stack objects. – Subaudition

Also heap allocated memory means new or malloc allocated. Stack allocated means you did not dynamically allocate the memory via new nor malloc. – Subaudition

I would hope so. I hate to think I am throwing around terms that I do not understand. – Cosmonaut

Answer 2

No you should not delete this from the destructor. The destructor gets called because of a delete statement (or goes out of scope) and this would most likely lead to some sort of crash.

You also have a couple problems in the DoublyLinkedList desturctor. One, you delete temp then access temp after its been deleted. Second, the code will not actually delete the last element in the linked list.

Alleenallegation

Answer 3

Currently, your code would cause an access violation, since the second of the following lines clearly accesses freed memory:

delete temp;
temp = temp->next();

If you want to recursively delete the structure, you want something like this:

DoublyLinkedList::~DoublyLinkedList
{
    Node *temp = first();
    delete temp;
}

Node::~Node
{
   if(this->next() != NULL) delete this->next();
}

Ardussi

A problem with this approach is that if you wanted to delete an arbitrary node, you'd wipe out every node that follows it. – Directly

Good point, thanks. I want to avoid a recursive implementation because that could get inefficient for large lists, I believe. – Cosmonaut

The OP's code will not necessarily cause AV - the freed memory may be left accessible by the process or it may be returned to the operating system and become inaccessible - it's up to the heap manager. – Bereft

@harto: That's easy to avoid by unlinking next() before destroying this. (If you have a doubly linked list, this seems more clean anyway, because otherwise next()->prev() would point to a deleted node.) – Chameleon

@Hooked: I'd measure before I'd say that. With linked lists, the non-locality (if that's a word) is probably the main performance hit. On top of that, recursion vs. iteration might not make any difference at all. IMHO the recursive approach fits natural and seems easily understandable, so before I'd forgo that, I'd measure. – Chameleon

@Dimitry: Your if(this->next() != NULL) delete this->next(); is unnecessarily verbose. It's OK to pass a NULL pointer to delete, so delete this->next(); will suffice. Also, DoublyLinkedList::~DoublyLinkedList() (besides missing ()) could be shorter, too, without losing (and maybe even gaining) readability. See my posting. :) – Chameleon

Answer 4

Before anything else: I really, really hope this is homework assigned to you in order to understand a doubly linked list. Otherwise there is no reason to use this instead of std::list. With this out of the way:

No, delete this in a dtor is always wrong, as the dtor is called when this is in the state of being deleted.

Also, while

delete temp;
temp = temp->next();

incidentally might work, it's certainly wrong, since, where you attempt to access temp->next(), temp is already deleted, so you should call a member function on it. Doing so invokes so-called "undefined behavior". (In short: It might do what you want, but it might just as well fail always or sporadically or only when Friday, 13th, collides with new moon. It migh also invoke very nasty nasal demons on you.)

Note that you could solve both problems by deleting the next node in your node's dtor:

Node::~Node()
{
   delete next();
}

That way, your list dtor becomes very easy, too:

DoublyLinkedList::~DoublyLinkedList()
{
    delete first();
}

To me, this seems what dtors were invented for, so, except for the fact that nobody should write their own linked list types anymore nowadays, to me this seem to be the C++ solution to your problem.

Chameleon

Not homework, but it is a learning exercise. I've never actually used a list. I am a fan of the STL, so don't worry about that. – Cosmonaut

@Hooked: A sigh of relieve on this end. :^> – Chameleon

Answer 5

delete this; would call the destructor of the current object. In that case, if you are calling delete this; in the destructor, then the destructor would be called infinitely till the crash.

Underwent

Answer 6

Code above will call Node::~Node() twice. (in "delete temp" and in Node::~Node())

Node::~Node() should not call "delete this" (or your program will crash)

ps. while loop in your code will not work. It will dereference invalid pointer. You should first copy the value of temp->next, and then destroy temp pointer.

Exieexigency

Actually, Node::~Node() won't be called twice, but end in a stack overflow (Ha!) due to endless recursion. – Chameleon

Answer 7

In general, a destructor should just worry about delete-ing (or free-ing if you're using C or malloc) any memory allocated specifically for your object. Deleting a pointer to your object will always be managed by the OS and you don't have to worry about a thing about that part.

One thing worth keeping in mind is that, on construction, you create the object first (as control flow enters the constructor's body), THEN the objects inside; for destruction, you have to do that in reverse, because if you delete the outer object first you would have no way to access the inner pointers to delete those. Instead, you delete inner objects using the destructor, then the OS manages the actual freeing up of memory when control flow falls out of the destructor.

Incidentally, the same sort of thing happens with subclassing-- if you have class A, and class B : public A, then when you do a new B(), the A constructor executes first, then the B's constructor; on destruction, the B's destructor executes first, followed by A's. I'm fairly sure that you do NOT have to worry about this, though-- C++ takes care of it for you. So don't try to figure out a way to call delete on the superclass.

Soelch

Answer 8

Both should never be done.

This

DoublyLinkedList::~DoublyLinkedList
{
    Node *temp = first();
    while (temp->next() != NULL)
    {
        delete temp;
        temp = temp->next();
    }
}

will cause undefined behaviour - you're not allowed to access memory that you've returned to the heap. Instead it shoud be:

DoublyLinkedList::~DoublyLinkedList
{
    Node *temp = first();
    while( temp != NULL)
    {
        Node* next = temp->next();
        delete temp;
        temp = next;
    }
}

Calling delete this will lead to so-called double free which will also cause undefined behaviour. The destructor should only call delete for pointer member variables, never for this. Calling delete this is reasonable from other methods to deallocate the current object, but not from the destructor.

Bereft