What does 'has virtual method ... but non-virtual destructor' warning mean during C++ compilation?

Question

#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(){}
  };

Woolpack

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. – Nawrocki

You mean class CRectangle: public CPolygon { public: virtual int area () { return (width * height); } }; ? – Woolpack

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. – Nawrocki

@Problemania why is there a semicolon in your example here: virtual ~CPolygon(){}; Meanwhile @Nawrocki does not have the semicolon in the above example? – Enunciation

@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. – Nawrocki

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? – Enunciation

Answer 1

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.

Glia

This answer would be greatly improved with an example demonstrating the ill effects of slicing. It will get an upvote from me when it has it. – Nawrocki

@Omnifarious: I added an example. – Hasheem

Just to be clear: delete foo invokes undefined behaviour, it is not guaranteed to only run ~FooBase. – Scofflaw

@Nawrocki What does "slicing" mean in this context? – Guanabara

Answer 2

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.

Burnight

It is only UB if you delete a sub-class object through a base-class reference or pointer. – Hasheem

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. – Cheek

From what I've heard, it mainly leads to problems if you (or someone) decides to extend off of that struct. – Continuum

Answer 3

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.

Inpour

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. – Kinky

@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. – Inpour

@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. – Cheek

@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. – Inpour

@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. – Cheek

@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 – Inpour

@EmilioGaravaglia: The answer is to not derive things from ::std::string. I don't know what purpose there would be in it anyway. – Nawrocki

@Omnifarious: hummm ... you don't know, but you have an answer. That's exactly what I call "religion". – Inpour

@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. – Cheek

@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. – Inpour

@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. – Cheek