Make Destructors virtual in GFXcanvas* classes

[michaelkamprath]

All this change does is make virtual the destructors declared in the GFXcanvas* classes. This is a simple fix. C++ class destructors should really be virtual to enable subclasses. The only reason why non-virtual class destructors should be used is if the class should never be inherited from. I am assuming it is valid for GFXcanvas* classes to be a base class (I can't imagine a reason why they shouldn't be base classes, but please inform me if there is). For people who do create a subclass from a GFXcanvas* class, a non-virtual destructor creates a really hard to discover bug (as I have learned). So, this change fixes the bugs that anybody who created a sub-class from a GFXcanvas* class is experiencing. Admittedly the bug is rarely expressed because most often in Arduino-style embedded programming objects don't get deleted. But, if you should ever need to delete an object derived from a class with a non-virtual destructor, the bug will manifest.

[BillyDonahue]

As a different solution to the subtle undefined behavior you're guarding against, we can instead mark the destructor as protected but keep it nonvirtual. This would follow Herb Sutter's classic advice (Sutter's Mill #18) on the issue.

Guideline #4: A base class destructor should be either public and virtual, or protected and nonvirtual.

This should probably be done at the root of the hierarchy, Adafruit_GFX, but also in the derived Canvas classes if they are going to be derived from as you propose.

It's almost academic, but I wanted to take some issue with this statement:

"The only reason why non-virtual class destructors should be used is if the class should never be inherited from."

That's a broad statement, and isn't generally true. There are base classes with polymorphic deletion, and there are base classes without it. Both are okay. Adding virtual dispatch to a destructor isn't free, and generates a little more code than a non-virtual destructor would. Space is at a premium, so we should be very conservative about emitting extra code we won't need.

[michaelkamprath]

As a different solution to the subtle undefined behavior you're guarding against, we can instead mark the destructor as protected but keep it nonvirtual. This would follow Herb Sutter's classic advice (Sutter's Mill #18) on the issue.

Guideline #4: A base class destructor should be either public and virtual, or protected and nonvirtual. This should probably be done at the root of the hierarchy, Adafruit_GFX, but also in the derived Canvas classes if they are going to be derived from as you propose.

To further quote Herb Sutter (because the above quote is lacking full context):

Don't derive from concrete classes.

So this is one design philosophy. There are others out there too. I'm sure I've used them all over the years. But this is not a language rule. The language rule is that it is undefined on how to handle non-virtual destructors through inheritance. In practice, this means that if you have non-virtual destructors, it forces users of the class to do the following:

• Never handle the class polymorphically. You have to delete the class using the same type it was created as.
• Manually chain call parent class destructors from child class destructors. Most compilers implement the non-defined behavior of handling non-virtual destructors and inheritance by calling only the destructor of the class type that delete was called on.

The better way to implement this design philosophy of preventing inheritance (regardless of the reason), per Stroustrup himself, is to make a class a final. Hiding the destructor under protected doesn't embrace the language's facility for preventing inheritance. Furthermore, Stroustrup says:

So when should I declare a destructor virtual? Whenever the class has at least one virtual function. Having virtual functions indicate that a class is meant to act as an interface to derived classes, and when it is, an object of a derived class may be destroyed through a pointer to the base.

Given Stroustrup's design philosophy, because Adafruit_GFX is a pure virtual class, all children of it, which GFXcanvas* classes are, should have virtual destructors regardless of intent as to whether you want people to inherit from it or not.

So, to me, it really comes down to whether you want to prevent people from inheriting from the class or not. If you want to prevent it, I would ask why, but it would explain some design choices. If you aren't intending to prevent people from inheriting from the class (they are awfully useful image buffer managers for making device drivers), then you really should make the destructor virtual.

It's almost academic, but I wanted to take some issue with this statement:

"The only reason why non-virtual class destructors should be used is if the class should never be inherited from." That's a broad statement, and isn't generally true. There are base classes with polymorphic deletion, and there are base classes without it. Both are okay.

They are both OK under different circumstances. The cases for non-polymorphic deletion is so rare and the bugs caused by not implementing polymorphic deletion when it is actually needed are so hard to debug, that it is usually better to "just make it virtual".

Adding virtual dispatch to a destructor isn't free, and generates a little more code than a non-virtual destructor would. Space is at a premium, so we should be very conservative about emitting extra code we won't need.

Adafruit_GFX is already a pure virtual class and any class derived from it already is embracing the cost of virtual methods. Adding one more method to the vtable is of marginal cost. Furthermore, per Stroustrup's rule, the destructor really should be virtual.

[BillyDonahue]

Ah, you're right. Withdrawn. These Canvas classes are already concrete AND polymorphic base classes. So the damage is done. Making their destructor protected would make them unusable as concrete object types.

I was concerned not with the vtable size, but that the calls to the destructor would occupy more instruction space and not be optimizable if it was made virtual. I did a little experimental build with and without virtual and didn't see a difference in compiled AVR code size.

Sorry for the noise.

[michaelkamprath]

Ah yes, code size would probably be more of a concern for my other PR #315 .

[michaelkamprath]

I was all excited that my PR got approved, but then who is this @blaquezzz? I see in his profile that his account was created 3 days ago and has done nothing but "approve" random PRs across seemingly unrelated repositories. sigh