Back to For Beginners

Problem with destructors in C++

For Beginners

Started by sooner123 December 02, 2012 07:34 PM

8 comments, last by iMalc 11 years, 4 months ago

sooner123

269

Author

December 02, 2012 07:34 PM

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.



  ~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();

  }

Endurion

5,545

December 02, 2012 07:50 PM

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;

   }

Fruny: Ftagn! Ia! Ia! std::time_put_byname! Mglui naflftagn std::codecvt eY'ha-nthlei!,char,mbstate_t>

MichaBen

481

December 02, 2012 07:51 PM

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.

sooner123

269

Author

December 02, 2012 08:16 PM

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.

rip-off

11,000

December 02, 2012 08:42 PM

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.

blog | twitter

SiCrane

11,840

December 02, 2012 09:56 PM

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.

kunos

2,256

December 03, 2012 06:34 AM

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> .

Stefano Casillo
TWITTER: [twitter]KunosStefano[/twitter]
AssettoCorsa - netKar PRO - Kunos Simulazioni

vdaras

293

December 03, 2012 02:46 PM

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.

rip-off

11,000

December 03, 2012 03:47 PM

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".

blog | twitter

iMalc

2,466

December 04, 2012 07:36 AM

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/

"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

Problem with destructors in C++

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Problem with destructors in C++

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines