Jump to content

  • Log In with Google      Sign In   
  • Create Account

Problem with destructors in C++


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
9 replies to this topic

#1 sooner123   Members   -  Reputation: 241

Like
0Likes
Like

Posted 02 December 2012 - 01: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();
  }

Edited by sooner123, 02 December 2012 - 02:44 PM.


Sponsor:

#2 Endurion   Crossbones+   -  Reputation: 3586

Like
1Likes
Like

Posted 02 December 2012 - 01: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;
   }

Edited by Endurion, 02 December 2012 - 02:14 PM.

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

#3 MichaBen   Members   -  Reputation: 481

Like
2Likes
Like

Posted 02 December 2012 - 01: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.

#4 sooner123   Members   -  Reputation: 241

Like
1Likes
Like

Posted 02 December 2012 - 02: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.

#5 rip-off   Moderators   -  Reputation: 8348

Like
3Likes
Like

Posted 02 December 2012 - 02:42 PM

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

For mature, popular languages, compiler bugs are vanishingly rare. Always blame your code first.

#6 SiCrane   Moderators   -  Reputation: 9604

Like
0Likes
Like

Posted 02 December 2012 - 03: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.

#7 kunos   Crossbones+   -  Reputation: 2207

Like
2Likes
Like

Posted 03 December 2012 - 12:34 AM

Holy father of leakage.. what do we have here? Posted Image
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, 03 December 2012 - 12:36 AM.

Stefano Casillo
Lead Programmer
TWITTER: @KunosStefano
AssettoCorsa - netKar PRO - Kunos Simulazioni

#8 vdaras   Members   -  Reputation: 237

Like
1Likes
Like

Posted 03 December 2012 - 08:46 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]

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, 03 December 2012 - 08:48 AM.


#9 rip-off   Moderators   -  Reputation: 8348

Like
1Likes
Like

Posted 03 December 2012 - 09:47 AM

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

#10 iMalc   Crossbones+   -  Reputation: 2306

Like
0Likes
Like

Posted 04 December 2012 - 01: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




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS