Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class does not have a virtual destructor #369

Open
TD-er opened this issue Nov 5, 2021 · 2 comments
Open

Class does not have a virtual destructor #369

TD-er opened this issue Nov 5, 2021 · 2 comments

Comments

@TD-er
Copy link

TD-er commented Nov 5, 2021

There is a number of libraries inheriting from the class in this library.
For example:

class Adafruit_SPITFT : public Adafruit_GFX

Which by itself is also inherited by other libraries, like this one:

class Adafruit_ILI9341 : public Adafruit_SPITFT

Neither Adafruit_SPITFT, nor Adafruit_GFX have virtual destructors.
Thus you cannot safely call delete on a base pointer to an instance of such class as it will cause a memory leak.
See also: StackOverflow - When to use virtual destructors?

I would really like these to have a virtual destructor.
But I can imagine it may add a slight overhead in some situations, which may be an issue on some micro controller use cases.

Would it be an idea to allow for some generic solution, to be used in all Adafruit libraries, to have this virtual destructor optional at compile time?

For example something like this:

#ifdef ADAFRUIT_NO_VIRTUAL_DESTRUCTOR
#define ADAFRUIT_VDTOR
#else
#define ADAFRUIT_VDTOR virtual
#endif

  ADAFRUIT_VDTOR ~Adafruit_SPITFT(){};

This should then be done in the header files of all library classes which will be inherited from.
Thus in this example at least in Adafruit_SPITFT and Adafruit_GFX

@BillyDonahue
Copy link
Contributor

A class automatically has a virtual destructor if any of its base classes does. So you wouldn't have to use this macro for ~Adafruit_SPITFT if ~Adafruit_GFX was already virtual. But I wouldn't want that destructor to be virtual.

GFX objects are rarely deleted at all. Usually they just sit at the top of a sketch as globals. So users don't typically run into this base pointer deletion problem. No libraries delete GFX pointers. They actually can't because it's an abstract base class. So any GFX user who wants a polymorphic delete will be doing it in application code. So they can make a class derived from some GFX class, give it a virtual destructor, and they'd use that in their delete statements?

@TD-er
Copy link
Author

TD-er commented Nov 6, 2021

The Adafruit_SPITFT class does have a non-compiler generated destructor, so it would be really strange if it will generate one with a different signature than defined in the code.

And in ESPEasy I do allow for these classes to be constructed and destructed in runtime as it is used in a plugin which can be enabled or disabled.
So the argument for "it is rarely being used" is not a valid argument to not implement it in a correct manner.
Don't get me wrong, I do understand that there are use cases where you might not want a virtual destructor, so that's why I suggested the defines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants