Public Group

# Problem with destructors in C++

This topic is 2053 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Here is my code. I'm having two issues with destructors here.

I'm trying to write a destructor that will run through the linked list of children of the current node being destroyed and delete all of them (which should recursively call their destructors as well)

The other issue is that this if I uncomment the code in RemoveAndDeleteAllWidgets, and try to manually call the destructor, I get an error saying 'no match for 'operator~' in '~Widget()'

This last error is making me suspect that my compiler might be buggy and have issues dealing with destructors.

 ~Widget() { cout << "children of node with value " << x << " being deleted" << endl; Node*sentinel = new Node; sentinel = list; cout << sentinel->data->x << "*" << endl; while (sentinel != NULL) { sentinel = sentinel->next; delete sentinel->prev; } } void RemoveAndDeleteAllWidgets() { //~Widget(); }  Edited by sooner123

##### Share on other sites
I don't want to sound harsh, but seriously, use std::list. Your code is leaking all over.
No, your compiler is surely not buggy. You don't call destructors directly.

Have you used the debugger and checked where it crashes?

The crash comes probably from that loop. Think about it, you're reaching the last element, so sentinel = sentinel->next; is setting sentinel to NULL.
And then you try to access sentinel->prev. Null pointer access!

 sentinel = list; cout << sentinel->data->x << "*" << endl; while (sentinel != NULL) { sentinel = sentinel->next; delete sentinel->prev; }  Edited by Endurion

##### Share on other sites
The first crash is probably because you assign sentinel to sentinel->next and after that call delete sentinel->prev. This will work, except for the last one, when sentinel->next is NULL and you try to delete NULL->prev. You did see correct that you need to get next before deleting, because after calling delete you cannot access next anymore, but you should use a local variable here, like
Node *next = sentinel->next; delete sentinel; sentinel = next;
And while it won't affect the crash, you call Node*sentinel = new Node, and then overwtite sentinel, leaking the memory. Probably not what you want.

About the second problem, calling a destructor yourself is evil, you should really try to avoid that. It's better to let your destructor call RemoveAndDeleteAllWidgets() instead if they share the same functionality. While there are ways to call a destructor explicitly, it's best to try working around it.

##### Share on other sites
Not harsh at all. I appreciate the response. And though I agree that my code is pretty much leaky garbage, I'd like to fine tune this and turn it into something that isn't garbage before I start using libraries to do this stuff for me.

And thank you for pointing out the error in the logic that was causing the crash.

One last mistake on my part. I was deleting the Node instead of the Class the Node pointed to.

##### Share on other sites

This last error is making me suspect that my compiler might be buggy and have issues dealing with destructors.
[/quote]
For mature, popular languages, compiler bugs are vanishingly rare. Always blame your code first.

##### Share on other sites
Note that calling new in a destructor is a bad idea, as is any operation that can cause an exception. Many of the C++ mechanisms for ensuring code correctness get thrown out the window if your destructors can throw, so at the minimum you need to trap those exceptions. But it's much better not to get into a situation where your operations can throw in the first place.

##### Share on other sites
Holy father of leakage.. what do we have here?
My comments in bold, cause I am bald... no correction, they are in RED cause the code tag isn't bold enough to show bold.

 ~Widget() { cout << "children of node with value " << x << " being deleted" << endl; Node*sentinel = new Node; // WHOA?? new called in a destructor? for what? sentinel = list; // AH! That's why called new! To leak it one line later. Good job! cout << sentinel->data->x << "*" << endl; while (sentinel != NULL) { sentinel = sentinel->next; // POTENTIALLY next is nullptr and if it is nullptr delete sentinel->prev; // BAM, deferencing a null pointer... CRASH } } void RemoveAndDeleteAllWidgets() { //~Widget(); } 

So, a total mess in one function, I can't imagine what else you have around in your code.
If you are learning that's all fine.. take your time and crash the entire world until you get it right.
If you are in production, do yourself a favour and #include <vector> . Edited by kunos

##### Share on other sites
My bet is that sooner123 doesn't understand the pointer concept very well.

sooner123,

[source lang="cpp"]Node* sentinel = new Node;[/source]

This line of code creates a new object of the Node class on the free store and makes the sentinel pointer reference that object.

So in order to make your pointer reference the pre-existing Node object, which you wanted to do in the first place, you just need this

[source lang="cpp"]Node* sentinel = list;[/source]

Sorry for stating the obvious to the vast majority of you, but from what I got from sooner123's responses, the op probably didn't understand why allocating the
new node was unnecessary. Edited by vdaras

##### Share on other sites
Note that "sentinel" is a poor variable name. When used with linked lists, the term "sentinel" means a special node value, which allows one to establish not-null invariants on the next or previous pointers.

A simpler and more accurate name might be "node" or "current".

##### Share on other sites

My bet is that sooner123 doesn't understand the pointer concept very well.

sooner123,

[source lang="cpp"]Node* sentinel = new Node;[/source]

This line of code creates a new object of the Node class on the free store and makes the sentinel pointer reference that object.

So in order to make your pointer reference the pre-existing Node object, which you wanted to do in the first place, you just need this

[source lang="cpp"]Node* sentinel = list;[/source]
I completely agree.
It very much demonstrates an incomplete knowledge of pointers and dynamic memory allocation, which being the case means that the destructor is bound to be just one of meny problems with the code, most of which has not been shown.

I would advise that you get yourself out of the mindset that if you fix this "last error" that you're even half way towards having a good linked-list class. That simply wont be the case at this point unfortunately. Learning is a process. You might have a good understanding of what it takes to make a good linked-list by the time you've written 5 different variations on them, leant all the ins and outs of using an existing linked-list class or two, and learnt all about exception safety. I highly recommend the Guru of the week site: http://www.gotw.ca/gotw/

1. 1
Rutin
25
2. 2
JoeJ
20
3. 3
4. 4
5. 5

• 9
• 9
• 46
• 41
• 23
• ### Forum Statistics

• Total Topics
631749
• Total Posts
3002053
×