51
#include <iostream>
using namespace std;

class CPolygon {
  protected:
    int width, height;
  public:
    virtual int area ()
      { return (0); }
  };

class CRectangle: public CPolygon {
  public:
    int area () { return (width * height); }
  };

Has compilation warning

Class '[C@1a9e0f7' has virtual method 'area' but non-virtual destructor

How to understand this warning and how to improve the code?

[EDIT] is this version correct now? (Trying to give answer to elucidate myself with the concept)

#include <iostream>
using namespace std;

class CPolygon {
  protected:
    int width, height;
  public:
    virtual ~CPolygon(){};
    virtual int area ()
      { return (0); }
  };

class CRectangle: public CPolygon {
  public:
    int area () { return (width * height); }
    ~CRectangle(){}
  };
qazwsx
  • 25,536
  • 30
  • 72
  • 106
  • Yes, the new version is correct. Though it's considered good form to re-declare functions in derived classes as virtual even though it's not necessary. This is so that people who just want to look at the derived class still know the functions are virtual. – Omnifarious Jan 07 '12 at 19:37
  • You mean `class CRectangle: public CPolygon { public: virtual int area () { return (width * height); } };` ? – qazwsx Jan 07 '12 at 22:02
  • Yes. And `virtual ~CRectangle() {}` as well. As I said, restating that these functions are virtual is simply good form, it's not required by the language in any way. – Omnifarious Jan 08 '12 at 03:37
  • 1
    @Problemania why is there a semicolon in your example here: `virtual ~CPolygon(){};` Meanwhile @Omnifarious does not have the semicolon in the above example? – CommaToast Sep 20 '12 at 19:17
  • 1
    @CommaToast: The `;` is totally superfluous. All by itself, it's just an empty statement. Sometimes you want an empty statement as the body of a `while` or `for` loop where everything is done with side-effects. I've never seen one used in the middle of a declaration, and I'm certain that it's inclusion was accident or confusion. – Omnifarious Sep 20 '12 at 22:48
  • So the `;` does no harm, but also does no good. A chaotic neutral semicolon? Maybe it's because he formerly had `virtual ~CPolygon()=0;` — which seems to be another common form of the virtual destructor (other than the Stay Puft marshmallow man). I'm not clear whether the =0 version is really the same. It's the "pure" kind right? – CommaToast Sep 21 '12 at 00:49

3 Answers3

93

If a class has a virtual method, that means you want other classes to inherit from it. These classes could be destroyed through a base-class-reference or pointer, but this would only work if the base-class has a virtual destructor. If you have a class that is supposed to be usable polymorphically, it should also be deletable polymorphically.

This question is also answered in depth here. The following is a complete example program that demonstrates the effect:

#include <iostream>

class FooBase {
public:
    ~FooBase() { std::cout << "Destructor of FooBase" << std::endl; }
};

class Foo : public FooBase {
public:
    ~Foo() { std::cout << "Destructor of Foo" << std::endl; }
};

class BarBase {
public:
    virtual ~BarBase() { std::cout << "Destructor of BarBase" << std::endl; }
};

class Bar : public BarBase {
public:
    ~Bar() { std::cout << "Destructor of Bar" << std::endl; }
};

int main() {
    FooBase * foo = new Foo;
    delete foo; // deletes only FooBase-part of Foo-object;

    BarBase * bar = new Bar;
    delete bar; // deletes complete object
}

Output:

Destructor of FooBase
Destructor of Bar
Destructor of BarBase

Note that delete bar; causes both destructors, ~Bar and ~BarBase, to be called, while delete foo; only calls ~FooBase. The latter is even undefined behavior, so that effect is not guaranteed.

Community
  • 1
  • 1
Björn Pollex
  • 75,346
  • 28
  • 201
  • 283
19

it means you need a virtual destructor on a base class with virtual methods.

struct Foo {
  virtual ~Foo() {}
  virtual void bar() = 0;
};

Leaving it off is can lead to undefined behavior, usually shows up as a memory leak in tools like valgrind.

Tom Kerr
  • 10,444
  • 2
  • 30
  • 46
  • It is only UB if you delete a sub-class object through a base-class reference or pointer. – Björn Pollex Jan 06 '12 at 20:58
  • 3
    Leaving it off is not - in and of itself - undefined behavior. The only undefined behavior that it might cause is if a dynamically allocated object of a derived type is deallocated with a `delete` expression where the type of the operand is of pointer to base class where the base class doesn't have a virtual destructor. There are other options for encourage safe usage such as giving the class a protected non-virtual destructor. – CB Bailey Jan 06 '12 at 20:58
  • From what I've heard, it mainly leads to problems if you (or someone) decides to extend off of that struct. – Chef Pharaoh Aug 23 '13 at 16:44
2

It merely means that a code like

CPolygon* p = new CRectangle;
delete p;

... or whatever wrapping into whatever smart pointer, will essentially not behave correctly since CPolygon is not polymorphic on deletion, and the CRectange part will not be destroyed properly.

If you're not going to delete CRectangle and CPolygon polymorphicaly, that warning is not meaningful.

Emilio Garavaglia
  • 20,229
  • 2
  • 46
  • 63
  • 2
    But if you're not going to delete CRectangle and CPolygon polymorphicaly, the base class destructor should be protected to enforce this at compile time. – Mark B Jan 06 '12 at 21:04
  • @MarkB: Not necessarilly: If CPolygon is not abstract (I don't know the OP abstraction how deep is), both CRect and CPolygon can be true perfect legal citizen of the stack, participating in CPolygon algorithms through references. The area needs to be calculated polymorphically, but no polymorphic destruction is needed. And CPolygon is required to be itself destructible (no protected destructor). Deriving a class that doesn't have a virtual destructor is not different than deriving a class that doesn't have ALL methods virtual. Simply don't expect what is not virtual to behave virtually. – Emilio Garavaglia Jan 06 '12 at 21:13
  • @EmilioGaravaglia: It's normally recommended to avoid deriving from a concrete class. It's too open for unintentional slicing in a variety of different guises. – CB Bailey Jan 06 '12 at 21:22
  • @CharlesBailey: yes, as it is normally recommended to don't use goto-s, that are recommended to break a double loop. But I don't see compiler warnings for those. Nothing itself bad in it, but don't make recommendation dogmas or religions. Recommendation are make to be not used, once understood what they are for. – Emilio Garavaglia Jan 06 '12 at 21:27
  • @EmilioGaravaglia: I wasn't making a recommendation, I was stating a common recommendation and I also gave the rationale for the recommendation. It's a recommendation that I agree with based on my experience and judgement and has no basis in dogma or religion. I just don't see how that is relevant. – CB Bailey Jan 06 '12 at 21:32
  • @CharlesBailey: To me, it's a recommendation I don't agree based on my experience that doesn't necessarily be the same as yours. I saw too many times std::string wrapped and it's entire interface rewritten because ... "it doesn't have a virtual destructor", when a simple derivation will suffice that I consider a warning like that even "dangerous" since it deformates many programmer's thinking. If fizz is virtual and buzz not, do you expect calling buzz something polymorphic? So why should be a destructor that different? If the destructor isn't virtual, just don't new/delete the object – Emilio Garavaglia Jan 06 '12 at 21:40
  • @EmilioGaravaglia: The answer is to not derive things from `::std::string`. I don't know what purpose there would be in it anyway. – Omnifarious Jan 06 '12 at 21:43
  • @Omnifarious: hummm ... you don't know, but you have an answer. That's exactly what I call "religion". – Emilio Garavaglia Jan 06 '12 at 21:48
  • @EmilioGaravaglia: OK, if you have developers that want to derive from `std::string` then I agree that you have more fundamental problems and I would agree that trying to educate them about the benefits of adding abstract base classes to a class hierarchy probably isn't going to be the first priority. – CB Bailey Jan 06 '12 at 22:08
  • @CharlesBailey: please be polite. You cannot AGREE something your own. I can interpret this as a personal insult. An I don't want to. If that is what your religion says, then be happy with your god, and let me be happy with mine. – Emilio Garavaglia Jan 07 '12 at 16:06
  • @EmilioGaravaglia: I'm sorry, I genuinely don't understand what you find insulting. I honestly thought I was agreeing with you and I have reviewed every one of my comments to this post. As far as I can tell, I have been scrupulously polite in every one and I am sorry that despite my efforts you have been able to interpret any form of personal insult or slight in any one of them. – CB Bailey Jan 07 '12 at 16:10