virtual deconstructor question
Moderator: Committer
virtual deconstructor question
I put in a fix for a memory leak in GG::TextControl (the recently added Font::RenderCache* was not not getting deleted upon destruction of the TextControl). I left it as a non-virtual since I didn't see any current need for it as virtual, but then after committing it I started rethinking that, since TextControl does get subclassed perhaps it would be safest/best to go ahead and make the deconstructor virtual. I decided to put the question to wiser heads...
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0
- Geoff the Medio
- Programming, Design, Admin
- Posts: 13587
- Joined: Wed Oct 08, 2003 1:33 am
- Location: Munich
Re: virtual deconstructor question
There are lots of explanations of the issue on the internet.
http://stackoverflow.com/questions/4612 ... estructors
In one of the answers: "...if a class has any virtual function, it should have a virtual destructor...".
In this case, it probably should be virtual, since one could store a pointer to a derived class object in a base class pointer and delete via that base class pointer. (Assuming I understand...)
http://stackoverflow.com/questions/4612 ... estructors
In one of the answers: "...if a class has any virtual function, it should have a virtual destructor...".
In this case, it probably should be virtual, since one could store a pointer to a derived class object in a base class pointer and delete via that base class pointer. (Assuming I understand...)
Re: virtual deconstructor question
Yes, there are a lot of explanations, but not all quite in full agreement about what choice to make when, though they mostly do advocate for doing *something* different than what I did , either a virtual public destructor or a protected destructor. Anyways, at looking into this a little more it appears it was a false choice, in that TextControl derives ultimately from Wnd, which has a virtual destructor and so the TextControl destructor is still virtual even if not explicitly declared so (to my understanding). So,Geoff the Medio wrote:There are lots of explanations of the issue on the internet. http://stackoverflow.com/questions/4612 ... estructors
In one of the answers: "...if a class has any virtual function, it should have a virtual destructor...". In this case, it probably should be virtual, since one could store a pointer to a derived class object in a base class pointer and delete via that base class pointer. (Assuming I understand...)
r7768 | dilvish-fo | 2014-11-14 09:22:33 -0800 (Fri, 14 Nov 2014) | 1 line
clarified that ~TextControl() is virtual
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0
Re: virtual deconstructor question
Looking at this just a little more, and thinking about the "Rule of Three", I'm wondering if instead of a raw pointer for TextControl::m_render_cache, perhaps a scoped_ptr or somesuch should be used instead so that an explicit/distinct deconstructor for TextControl isn't necessary at all. I'm not really up for going through the code to see what corresponding changes might be necessary, but thought I should mention the idea.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0
Re: virtual deconstructor question
Here is my general owned member pointer workflow:
Oh, this class owns this, but needs to store it as a pointer, so unique_ptr. We don't have unique_ptr. What's the closest thing? Scoped_ptr. Hmm... the lack of move semantics is creating annoying compilation issues. I'll just make it a raw pointer and handle it manually. Why is it crashing? Oh, I made one of the zillion possible subtle errors with raw pointers.
One solution is to use shared_ptr, but you lose a smidgeon of performance, and worse, you are telling the reader of the code that this here is a shared pointer, while it in fact is not shared at all.
To summarize, I can think of three options (in the absence of move semantics)
1) Use shared_ptr, it's easy. It's easy and safe. (Well, as safe as c++ gets)
2) Use scoped_ptr, use class in a way that avoids the issues or solve them. (With custom a copy constructor and assignment, I think, but the class will need to have a "out of commission" state for when it has relinquished its payload pointer to someone else. Or in some cases you could deep copy.)
3) Use a raw pointer. Make no mistakes.
This is definitely worth thinking about. Having a policy on the matter would be good.
Oh, this class owns this, but needs to store it as a pointer, so unique_ptr. We don't have unique_ptr. What's the closest thing? Scoped_ptr. Hmm... the lack of move semantics is creating annoying compilation issues. I'll just make it a raw pointer and handle it manually. Why is it crashing? Oh, I made one of the zillion possible subtle errors with raw pointers.
One solution is to use shared_ptr, but you lose a smidgeon of performance, and worse, you are telling the reader of the code that this here is a shared pointer, while it in fact is not shared at all.
To summarize, I can think of three options (in the absence of move semantics)
1) Use shared_ptr, it's easy. It's easy and safe. (Well, as safe as c++ gets)
2) Use scoped_ptr, use class in a way that avoids the issues or solve them. (With custom a copy constructor and assignment, I think, but the class will need to have a "out of commission" state for when it has relinquished its payload pointer to someone else. Or in some cases you could deep copy.)
3) Use a raw pointer. Make no mistakes.
This is definitely worth thinking about. Having a policy on the matter would be good.
Any code by me in this post is released under GPL 2.0 or later.