Polymorphic Destructor issue in ILI9341_t3 class

D

DraconisRex

Member
I'm working on writing an adapter class that will allow me to quickly plug-in different ILI9341 libraries.

I get a compilation warning in my wrapper class for the ILI9341_t3 class because it implements the Print interface, but does not provide a virtual destructor. In my day job all warnings are treated as errors, and I feel the same about code I write for myself. This really should be fixed since it transgresses into the realm of undefined behavior.

Code: 
C:\Users\SPC\Documents\Arduino\libraries\SPC\Teensy3ILI9341Adapter.cpp: In destructor 'virtual Teensy3ILI9341Adapter::~Teensy3ILI9341Adapter()':

C:\Users\SPC\Documents\Arduino\libraries\SPC\Teensy3ILI9341Adapter.cpp:8:10: warning: deleting object of polymorphic class type 'ILI9341_t3' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]

   delete _tft;

          ^
 
Last edited:
I may be missing something, but why does ILI9343_t3 need a virtual Destructor ? Yes, I know it implements the print interface, but the only virtual functions I see in Print class are two virtual write statements and I don't believe it has a destructor.
 
Hmm, I think the issue starts in Print.h. Since Print is an abstract class, due to the multiple pure virtual functions. As such, it should have a virtual destructor so that destroying an instance of Print by deleting a Print pointer will invoke the destructor of the appropriate subclass. Scott Meyers gives an extensive treatment of this topic in his book, Effective C++. It may in fact be this missing destructor that the warning is referring to, though it is unclear that is the meaning if that is the case. The fact that I'm freeing an ILI9341_t*, rather than a Print*, may be why this is only a warning. I took the compiler warning at its word rather the dig through the implementing classes when I started this thread.
 
Last edited:
Actually I overqualified the need for the virtual destructor. As Scott Meyers says, any polymorphic base class requires the virtual destructor in order to behave correctly when deleting an instance via a pointer to the base class.
 
You haven't posted your code, but the warning is probably bogus. If you use the correct type in the delete expression, no virtual destructor is required.
 
The warning is not bogus and I'm using the correct delete.

Teensy3ILI9341Adapter.h
Code: 
class Teensy3ILI9341Adapter : public TftAdapter {
 public:
  Teensy3ILI9341Adapter(uint8_t cs, uint8_t dc);
  Teensy3ILI9341Adapter(uint8_t cs, uint8_t dc, uint8_t rst, uint8_t mosi, uint8_t sclk, uint8_t miso);
  ~Teensy3ILI9341Adapter();

  ...

 private:
  ILI9341_t3*   _tft;
};


Teensy3ILI9341Adapter.cpp
Code: 
Teensy3ILI9341Adapter::Teensy3ILI9341Adapter(uint8_t cs, uint8_t dc)
    : _tft(new ILI9341_t3(cs, dc)) {}

Teensy3ILI9341Adapter::Teensy3ILI9341Adapter(uint8_t cs,
                                             uint8_t dc,
                                             uint8_t rst,
                                             uint8_t mosi,
                                             uint8_t sclk,
                                             uint8_t miso)
  : _tft(new ILI9341_t3(cs, dc, rst, mosi, sclk, miso)) {}

Teensy3ILI9341Adapter::~Teensy3ILI9341Adapter() {
  delete _tft;
}
 
Last edited:
Here is a complete sketch that will produce the warning.

Code: 
#include <ILI9341_t3.h>

class Adapter {
 private:
  ILI9341_t3*  instance;

 public:
  Adapter() : instance(new ILI9341_t3(0,1)){}
  ~Adapter() {
    delete instance;
  }
};

void setup() {
}

void loop() {
}
 
Last edited:
I've now confirmed that the issue is actually in Print.h. The sketch below will produce the warning. Uncommenting the virtual destructor resolves the warning. It was necessary to make the base class abstract by adding the pureVirtual() function in order to produce the warning.

Code: 
class BadBase {
 public:
  BadBase () {}
  // virtual ~BadBase () {}

  virtual void pureVirtual () = 0;
};

class BadSubClass : public BadBase {
 public:
  BadSubClass () {}

  void pureVirtual () {}
};

class BadSubClassAdapter {
 private:
  BadSubClass*  instance;

 public:
  BadSubClassAdapter() : instance(new BadSubClass){}
  ~BadSubClassAdapter() {
    delete instance;
  }
};

void setup() {
}

void loop() {
}
 
Last edited:
KurtE said:
I may be missing something, but why does ILI9343_t3 need a virtual Destructor ? Yes, I know it implements the print interface, but the only virtual functions I see in Print class are two virtual write statements and I don't believe it has a destructor.
Click to expand...

There is always a destructor even if it isn't explicitly defined.
 
tni said:
The warning is a false positive. The code you posted is completely valid and there is no undefined behavior.
Click to expand...

Yes, my code is safe and the correct destructors will be called, but the warning is telling us that there is a problem with the Print class and it should be fixed. It would be so easy to write a class that accepts a Print* that is expected to release its resources at some point, meaning it would delete the Print* object and in that case only the ~Print() destructor would be called. This is the issue the warning is trying to bring to our attention. Unfortunately creating that condition in code, as illustrated in the example code below, only raises the warning again, not an error. Taking the position that "It's only a warning" develops a habit of leaving dangerous code unaddressed, even if it isn't biting you in the ass at this time. Such issues should be addressed when identified, rather than waiting for actual problems to arise. This is precisely why my team uses the "treat all warnings as errors" switch for all our code at work, leaving us no choice but to generate code that compiles clean. I'd love to see that same switch enabled in Arduino code, but then I'd have to fix libraries. Since my employment prohibits me contributing to open source projects I can't do that. The best I can do is point out this issue in the hopes that somebody else will fix it. Even doing this pushes the bounds of the terms of my employment.

So this canary has stopped singing. The community can do what it wants as the mine continues to fill with the noxious gas of ignored warnings.

Code: 
class BadBase {
 public:
  BadBase () {}
  // virtual ~BadBase () {}

  virtual void pureVirtual () = 0;
};

class BadSubClass : public BadBase {
 public:
  BadSubClass () {}

  void pureVirtual () {}
};

class BadSubClassAdapter {
 private:
  [COLOR="#FF0000"]BadBase* instance;[/COLOR]

 public:
  BadSubClassAdapter() : instance(new BadSubClass){}
  ~BadSubClassAdapter() {
    delete instance;
  }
};

void setup() {
}

void loop() {
}
 
Last edited:
Print from Arduino doesn't have a virtual destructor.

Adding a virtual destructor is not a no-brainer, since it results in a non-trivial amount of code bloat in each and every class that inherits from Print. The compiler will be unable to de-virtualize the destructor calls in the majority of cases.

Making the Print destructor protected might be a good idea.
 
tni said:
Print from Arduino doesn't have a virtual destructor.

Adding a virtual destructor is not a no-brainer, since it results in a non-trivial amount of code bloat in each and every class that inherits from Print. The compiler will be unable to de-virtualize the destructor calls in the majority of cases.

Making the Print destructor protected might be a good idea.
Click to expand...

Print for Teensy3 is not the Print from Arduino, so what Arduino does or does not do is rather a moot point in this forum.

Yes, adding the virtual destructor to Print will result in additional code, but it is the only way to make Print a true abstract base class with guaranteed correct behavior. My testing, using the BadBase example, indicates this "bloat" is on the order of 100 bytes. I have not tested if this code is per subclass or not.

Writing bad abstract base classes imposes an assumption of use cases on the end users, an assumption that is not communicated anywhere in the Print code that I can see. I for one am not prescient and would not want to be responsible for making such assumptions about the uses of a library I create. Adding the protected destructor will prevent users from being able to make this error, but doing so would severely hamper the ability to write a PrintFactory class.

To be clear, I'd prefer to see the virtual destructor so that the Print class is fully featured, but lacking that I would definitely agree that the protected destructor should be added to prevent incorrect deletion via Print pointers. Unfortunately such addition does not eliminate the warning.
 
Last edited:
DraconisRex said:
Print for Teensy3 is not the Print from Arduino, so what Arduino does or does not do is rather a moot point in this forum.
Click to expand...
I don't think so. Compatibility matters and has been a very large concern for Teensy.
What value do you think would be added by making the destructor protected?
Click to expand...
Code: 
class A {
public:
    virtual void print();
protected:
    ~A();
};

class B : public A {
public:
    void print() override;
    ~B();
};

class C {
public:
    C() { b = new B(); }
    ~C() { delete b; }
    A* b;
};

Compile error:
test.cpp:5:5: error: ‘A::~A()’ is protected
     ~A();
     ^
test.cpp:17:19: error: within this context
     ~C() { delete b; }
                   ^

I'll still be able to convert a pointer to a subclass of Print into a pointer to Print and I'll still be able to call delete on that pointer and the Print::~Print() destructor will still be invoked. And it will still miss calling the appropriate destructor(s).
Click to expand...
Nope.
Yes, adding the virtual destructor to Print will result in additional code, but it is the only way to make Print a true abstract base class with guaranteed correct behavior. My testing, using the BadBase example, indicates this "bloat" is on the order of 100 bytes. I have not tested if this code is per subclass or not.
Click to expand...
A simple test sketch that only uses USB serial increases 400 bytes in size with a virtual "~Print() {}". That's a pretty high price for a feature that hardly anyone will use but everybody has to pay for.

People can work around the missing virtual destructor. E.g. std::shared_ptr will keep track of the proper deleter and works correctly with a base class pointer.

This code will work correctly:
Code: 
#include <memory>

class A {
public:
    virtual void print();
protected:
    ~A();
};

class B : public A {
public:
    void print() override;
    ~B();
};

class C {
public:
    C() { b = std::make_shared<B>(); }
    ~C() {}
    std::shared_ptr<A> b;
};
 
I withdrew my comments regarding the protected destructor, but obviously not before you started your response. It was something I've not run into previously, but on reflection I realized the construct didn't make a lot of sense, so maybe they'd given it some "special" behavior.
 
You must log in or register to reply here.
Back
Top