Sign in to follow this  
sooner123

Problem with destructors in C++

Recommended Posts

sooner123    269
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)

Instead the program just crashes.

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.

[code]
~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();
}
[/code] Edited by sooner123

Share this post


Link to post
Share on other sites
Endurion    5408
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!

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

Share this post


Link to post
Share on other sites
MichaBen    481
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
[code]Node *next = sentinel->next;
delete sentinel;
sentinel = next;[/code]
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 this post


Link to post
Share on other sites
sooner123    269
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 this post


Link to post
Share on other sites
rip-off    10976
[quote]
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 this post


Link to post
Share on other sites
SiCrane    11839
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 this post


Link to post
Share on other sites
kunos    2254
Holy father of leakage.. what do we have here? [img]http://public.gamedev.net//public/style_emoticons/default/biggrin.png[/img]
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.

[quote name='sooner123' timestamp='1354476891' post='5006350']
[code]
~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();
}
[/code]
[/quote]

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 this post


Link to post
Share on other sites
vdaras    293
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 [b]Node[/b] class on the free store and makes the sentinel pointer reference that object.

So in order to make your pointer reference the pre-existing [b]Node [/b]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 this post


Link to post
Share on other sites
rip-off    10976
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 this post


Link to post
Share on other sites
iMalc    2466
[quote name='vdaras' timestamp='1354545989' post='5006609']
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 [b]Node[/b] class on the free store and makes the sentinel pointer reference that object.

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

[source lang="cpp"]Node* sentinel = list;[/source][/quote]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 [i]using [/i]an existing linked-list class or two, and learnt all about exception safety. I highly recommend the Guru of the week site: [url="http://www.gotw.ca/gotw/"]http://www.gotw.ca/gotw/[/url]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this