virtual deconstructor question

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Post Reply
Message
Author
User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

virtual deconstructor question

#1 Post by Dilvish »

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

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: virtual deconstructor question

#2 Post by Geoff the Medio »

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

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: virtual deconstructor question

#3 Post by Dilvish »

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...)
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 :lol:, 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,
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

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: virtual deconstructor question

#4 Post by Dilvish »

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

Mitten.O
Programmer
Posts: 255
Joined: Sun Apr 06, 2014 4:15 pm

Re: virtual deconstructor question

#5 Post by Mitten.O »

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.
Any code by me in this post is released under GPL 2.0 or later.

Post Reply