Add virtual destructor to thread.h to prevent undefined behavior on destruction.

[DavidBoman]

According to the documentation of ArduinoThread one should be able to inherit from the Thread class:

"Inheriting from Thread or even ThreadController is always a good idea. For example, I always create base classes of sensors that extends Thread, so that I can "register" the sensors inside a ThreadController, and forget about really reading sensors, just getting theirs values within my main code. Checkout SensorThread example."

When doing this in Eclipse I get an error like "Class 'XXX' has virtual method 'run' but non-virtual destructor"

according to this:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#discussion-make-base-class-destructors-public-and-virtual-or-protected-and-nonvirtual

a base class should define a public virtual destructor otherwise destruction via a pointer to a base class would lead to undefined behavior (and a possible memory leak as the inherited destructor isn't guaranteed to be called).

...and a disclaimer: I'm originally a Java guy and has just recently started to learn C++. I might have gotten everything completely wrong :)

[CAHEK7]

I would say you are right, if it was a common case. Typically virtual destructor must be defined in case of polymorphic classes. But there can be the dragons.
First of all, the behavior is not truly "undefined", it is actually well defined - current type destructor will be called without any hierarchy and inheritance checks (if you destroy pointer to base class - base class destructor will be called, if you destroy pointer to derived class - derived class destructor will be called, no matter where pointer actually points - to base or to derived) and this may cause memory leaks in first case and truly "undefined" behavior in the second. Or may not.
It depends on what is the classes structure and API agreements, so it is possible to safely use polymorphic classes without virtual destructor.

But I agree, to make library bulletproof it would be nice to have virtual destructor for thread to avoid any misuse or errors in custom-defined derived classes.

[ivanseidel]

Will this add incompatibility of any kind? If I understand correctly, the purpose of this is to allow inherited classes (other than the core itself) to know when it got destcructed.

[DavidBoman]

Once again with the disclaimer that I'm a Java guy... but I don't think it will. My understanding is that it will only call a destructor if it exists. If no destructor exists nothing will happen. If a destructor does exists it will now be called when you destroy pointer to base class, not only when you destroy the pointer to the derived class. This is a changed behavior but I really can't see a case where you really would like to call the destructor for the derived class only but not when destroing via a base class.

[CAHEK7]

I'll clarify the things a little bit.

My understanding is that it will only call a destructor if it exists. If no destructor exists nothing will happen.

There is always a destructor. At least default one (generated automatically if no specific destructor defined in the class), it may appear it does nothing, but at least it calls all the desructors from all the class members. Destructor can be normal or virtual, the main difference between them - if you destroy a class without virtual destructor, the destructor of that class will be called (if you destroy a pointer to base class - only base class destructor will be called disregard to the fact that the pointer may point do some derived class). That is absolutely the same behavior if you call non-virtual function.
But if the destructor (or function) is virtual, if will determine (using vtable and inheritance) the actual class and call appropriate destructor form this particular class.

The drawback of non-virtual destructors - potential memory or resources leak. If you have a pointer to base class, but create derived class which allocates some additional resources for itself, only the base class destructor will be called and all the resources allocated in derived class will leak. Virtual destructors solve that problem.

This change won't introduce any incompatibility unless somebody use precompiled version of this library (which is very very unlikely). The same problem with compatibility with precompiled libraries may happen if you just introduce any new method.

There are no problems with compatibility if the project have always built from the sources.