2

There is a virtual class as a callback interface, that I can neither modify, nor ask the author to fix. The only members of the class are a lot of virtual methods that can be overridden, so as to let the library call back into my code. In order to get some callback opportunities, I should make a derived class of that virtual class, and override corresponding virtual methods. If I'm NOT interested in some callback chances, I just need to avoid overriding them.

But, the declaration of that interface class has a defect - its destructor is NOT declared as virtual.

For example:

class callback_t {
public:
  virtual void onData( int ) {};
};

I make a child class and do not override the destructor, but when I delete a dynamic object of the class child_t, I encounter a warning from the compiler (gcc9 with C++17) :

deleting object of polymorphic class type ‘child_t’ which has non-virtual destructor might cause undefined behavior.

class child_t : public callback_t {
public:
  ~child_t() {
    // release things specific to the child...
  };
  void onData( int ) override {
    // do things I want when onData
  };

private:
  int m_data = 0;
};

int main() {
  child_t* pc = new child_t;

  // pass pc into the routines of the library
  // working...

  delete pc;   /*deleting object of polymorphic class type ‘child_t’ which has non-virtual destructor might cause undefined behavior */
};

Question: How to correctly and gracefully eliminate the warnning(I must commit codes with no warnning)?

Notes and revising:

  1. I can NOT modify the declaration of class callback_t, I also can NOT ask the author of it to fix! This is a lib that had been released by an authority institution. It's useless to advice me change the base class, and it's no meaning to judge the code quality of the lib;

  2. I never intend to release objects of class child_t with a pointer of the type of the base class, and I known clearly the differenc between virtual-dtor and static-dtor;

  3. The main problem I need to resolve, is to eliminate the compiling warnning, because I'm sure there's no memory leaking, and no omission of restoring some states;

  4. In this case, there's no data, no meaningful codes in the base class, and the only intention of making it, is to be derived from, so it should NOT be marked final. But I tried making the child_t as 'final', and the warnning went away. I'm not sure this method is correct. If so, I think it is the cheapest methods so far;

  5. I also tried making the dtor of child_t as virtual, and the warnning went away also. But I am still not sure whether or not it is correct.

Leon
  • 1,489
  • 1
  • 12
  • 31
  • 2
    perhaps you are looking for this: https://stackoverflow.com/questions/3899790/shared-ptr-magic – 463035818_is_not_an_ai Jul 08 '20 at 15:18
  • I can think of two things: use only stack variables, and smart pointer, so no explicit memory deletion is needed, and if nothing needs to be changed in the destructor then you're good to go. If it's not the case, then isn't there any of those lots of virtual methods where you know that you don't need a certain thing or can set some state which otherwise would have been done in the destructor? Because then you could do it there, instead of the destructor. – Andrew Jul 08 '20 at 15:20
  • 1
    @Andrew "if nothing needs to be changed in the destructor" thats not the correct basis to decide if there is a problem. Either you run into undefined behavior or you don't. What the destructor actually does is not that relevant in either case – 463035818_is_not_an_ai Jul 08 '20 at 15:24
  • Only need a virtual destructor if you are going to delete the object through that base class pointer. For an interface class, that's not usual (although it is done that way sometimes). To be polite, the destructor for the interface class should have been made `protected:` to make that clear. – Eljay Jul 08 '20 at 16:06
  • Why are you doing this: `child_t* pc = new child_t;`? Can't you use `child_t child;` and pass `&child` to the "routines of the library"? – Tony Delroy Jul 11 '20 at 11:06
  • @TonyDelroy Thanks! It's another topic about "Why are you doing like child_t* pc = new...". I'm using "pimpl" idiom within my project, so in fact a `child_t* pc` is a member variable of another obj, and would be allocated dynamically. – Leon Jul 11 '20 at 12:44

2 Answers2

7

A virtual destructor is only needed if you need to delete derived objects via a base class pointer. If you don't need to do that, then the fact that the base class destructor is not virtual doesn't matter (although it's a fairly big hint that the class was never intended to be inherited from - in modern code it should probably have been marked final). You can still derive from the class just fine. You just have to be careful about how objects of the derived class are destroyed.

Jesper Juhl
  • 30,449
  • 3
  • 47
  • 70
  • Are you meaning that "I just need to make the destructor of derived class 'virtual'."? – Leon Jul 08 '20 at 16:34
  • 1
    @Leon "Are you meaning that "I just need to make the destructor of derived class 'virtual'."?" - No. That's not *at all* what I'm saying. I'm saying that if the base class destructor is not `virtual`, you can still derive from the class, but you need to be careful about how you `delete` derived objects, since without a `virtual` base class dtor, it will *not* be valid to `delete` a derived object via a pointer to the base class type - but deleting a derived object directly is fine. – Jesper Juhl Jul 08 '20 at 16:39
7

The warning you are getting is a false positive. With

child_t* pc = new child_t;

// pass pc into the routines of the library
// working...

delete pc;

pc points to a child_t, and it's static type is pointer to a child_t, so the correct destructor will be called. If you had

callback_t* pc = new child_t;

// pass pc into the routines of the library
// working...

delete pc;

Then the warning would be correct as only the callback_t destructor would be called.


There is a work around for this and that is to use a std::shared_ptr. The pointer stores the correct deleter in it's storage so even if the destructor is not virtual, the correct derived destructor is called instead of the base one. You can see more about this in shared_ptr magic :)

NathanOliver
  • 171,901
  • 28
  • 288
  • 402
  • Resolving it with smart pointer is an amazing methods, and I like it very much! But other answer offer some lower cost methods, such as "making derived class 'final'", and "making the dtor of child_t 'virtual'". Would you pls talk about which is the best in my case? – Leon Jul 08 '20 at 16:41
  • 1
    @Leon The other answer talking about final is saying that the person who wrote the class should have marked it as `final` to indicate you are not to derive from it. The other answer about making the destructor virtual was wrong and has been deleted. – NathanOliver Jul 08 '20 at 16:43
  • In my case, there's no data, no meaningful codes in the base class, and the only intention of making it, is to be derived from, so it can NOT be marked final. I tried making the child_t 'final', and the warnning went away. But I'm not sure this method is correct. – Leon Jul 08 '20 at 16:54
  • 1
    @Leon You either need to change `callback_t` to have a virtual destructor, use a `shared_ptr`, use a `std::variant` use the visitor pattern, or like you did use a pointer to the derived type. Anything else is going to have undefined behavior. – NathanOliver Jul 08 '20 at 16:56
  • I'll certainly make sure always call directly the dtor of the derived class. I just need to eliminate the warnning. – Leon Jul 08 '20 at 17:38