21

I came across a base class whose destructor is non-virtual, although the base class have 1 virtual function fv(). This base class also has many subclasses. Many of those subclasses define its own fv().

I don't know the details of how the base and subclasses are used in the program. I only know the program works fine even the destructor of base class should be virtual.

I want to change the destructor of base class from non-virtual to virtual. But I'm not sure of the consequences. So, what will happen? What more do I need to do to make sure the program works fine after I change it?

Follow up: After I changed the base class's destructor from non-virtual to virtual, the program failed one test case.
The result confuses me. Because if the destructor of base class is not virtual, then the program won't use base class polymorphicaly. Because if not, it leads to undefined behavior. For example, Base *pb = new Sub.
So, I think if I change destructor from non-virtual to virtual, it shouldn't cause any more bugs.

Yuan Wen
  • 1,343
  • 3
  • 17
  • 36
  • 1
    Have you tried it out? :D – qxz Aug 22 '16 at 07:51
  • 2
    Note that subclasses and virtual functions do not necessarily imply virtual destructors -- a virtual destructor is only needed if the objects are to be destructed polymorphically (through the static type of the base class). An automatic variable or an instance managed by a `shared_ptr`, for example, don't need it. – Quentin Aug 22 '16 at 07:53
  • Yes, I tried it. But unfortunately, the new program failed one test case.@qxz – Yuan Wen Aug 22 '16 at 07:53
  • What about the class' behavior changed? – qxz Aug 22 '16 at 07:53
  • 4
    i'd say you should always declare destructors as virtual in those classes which will be used as base classes. otherwise you'll probably get UB (that's UB - it's just like a time bomb, you can live for a while with it, but someday it explodes) – fgrdn Aug 22 '16 at 07:55
  • The program is too complicated for me to figure it out yet. Could you please offer some hint to it? @qxz – Yuan Wen Aug 22 '16 at 07:56
  • 6
    @fgrdn I would disagree. Not all base classes are designed to be used (and deleted) polymorphically, and not having a virtual destructor is a good indicator for this. I would suggest making the destructor protected in those cases though. – juanchopanza Aug 22 '16 at 07:57
  • 1
    Well, without a virtual destructor, deleting a `Base*` that points to a `Derived` will probably cause UB. So making it virtual would mean it (should) behave properly. – qxz Aug 22 '16 at 07:59
  • The original base class works fine. But changing destructor from non-virtual to virtual causes bugs.@qxz – Yuan Wen Aug 22 '16 at 08:01
  • 4
    The usual rule of thumb is that whenever you have a `virtual` method, almost always you want a `virtual` destructor (also, once you have a single virtual method the price of the vptr has already been paid, so adding an extra virtual method is essentially free). – Matteo Italia Aug 22 '16 at 08:01
  • 1
    @YuanWen Can you provide some example code (or a link to it), and more information on how exactly the program behaves differently? – qxz Aug 22 '16 at 08:04
  • 1
    @juanchopanza you never guarantee the future of these base classes. i mean serious big projects with a team working. i'd leave class without vtbl only if this class prevents using itself as base in future. – fgrdn Aug 22 '16 at 08:05
  • 3
    @YuanWen: that's scary. A well formed program shouldn't change behavior by adding `virtual` to a base class destructor. Anyhow, if that's of any use in your investigation, normally when you don't have a base class virtual destructor what happens is that, when you are deleting a derived object through a pointer to base, the derived destructor doesn't get called, and the fields added in the derived class aren't destroyed. – Matteo Italia Aug 22 '16 at 08:05
  • 1
    @fgrdn You can't guarantee anything, but you can specify how the class is to be used and you can protect against deleting from a base class by making the destructor protected. Note that I didn't say anything about leaving a vtbl out. I was only referring to the destructor being virtual. – juanchopanza Aug 22 '16 at 08:07
  • 1
    @juanchopanza indeed - you can't guarantee everything. but just imagine - you have base class, you protect its destructor. but then someone in your team make another class (let's say baseClassEnhancer) with public destructor, but without virtual keyword (just because he/she thinks - it's a base class, so it has to have its destructor virtual). and then make derived from this another class. of course you can talk with this teammate not to do it again, but before your app throw a bunch of exceptions – fgrdn Aug 22 '16 at 08:20
  • 4
    @fgrdn "just because he/she thinks - it's a base class, so it has to have its destructor virtual" OK, so you have to make the destructor virtual because there are coders out there who think you need to make the destructor virtual? Sounds like cargo cult programming to support cargo cult programmers. – juanchopanza Aug 22 '16 at 08:26
  • 2
    @fgdrn If that happens, you failed to document your code properly. Everybody wants to pretend C++ is sane enough to make your intent clear through code alone because blah blah philosophy, but it really isn't. If you don't want a class to be inherited from then leave a clear comment in the source instead of assuming that intent will be clearly communicated by the presence or absence of a virtual destructor alone. If you really want to do it right, also explain *why* in your documentation. – Jason C Aug 22 '16 at 08:32
  • @juanchopanza making destructor virtual isn't such a performance problem nowadays – fgrdn Aug 22 '16 at 08:33
  • @fgrdn Your happy subclasser will get a compile-time error when declaring the destructor `override`, as should be done whenever you assume a function is virtual. – Quentin Aug 22 '16 at 08:37
  • 1
    @jason-c you can find a lot of projects, libraries, frameworks, that are working great and they don't have documentation or have but not so good. only in ideal world everything is documented. – fgrdn Aug 22 '16 at 08:37
  • @Quentin if subclasser DOES it by himself. he can just leave destructor as `~b1 () { /* do stuff */ }` – fgrdn Aug 22 '16 at 08:42
  • 1
    @fgdrn "You find lot of projects that work great..." and therefore, what? That's inappropriately general. You identified *your* hypothetical team as a team that contains people who may make those mistakes. So for your team in your environment, it is a documentation problem. For a smaller team in a more controlled environment the definition of "proper" documentation may change. For situations besides the one we are talking about with virtual destructors and intent for polymorphism, it may change too. I'm talking about the conversation actually being had in comments right now. :) – Jason C Aug 22 '16 at 08:44
  • 3
    @fgrdn That's typically the point at which you require that developers actually know how to use their tools properly. – Quentin Aug 22 '16 at 08:49
  • 1
    Voting to close as the symptoms (the safe act of marking a destructor as `virtual` somehow introducing bugs) indicate UB, yet the OP has provided no MCVE or any code, thus speculating about it is useless. _"Questions seeking debugging help ("**why isn't this code working?**") must include the desired behavior, a_ specific problem or error _and_ the shortest code necessary _to reproduce it **in the question itself**. Questions without a clear problem statement are not useful to other readers. See: [How to create a Minimal, Complete, and Verifiable example](http://stackoverflow.com/help/mcve)."_ – underscore_d Aug 22 '16 at 09:35
  • 2
    @underscore_d Disagree. The question is clear, interesting, on-topic, well-written, and answerable, and received informative, useful, and quite sufficient answers. I concede that it may not be one of the usual [inane debugging questions](http://stackoverflow.com/questions/25826384/i-cant-figure-out-the-following-outputs-can-anyone-please-explain) that you're probably used to, which is refreshing. This question is, in fact, useful to other readers, and no universe exists in which the close reason you chose is applicable here (for starters, this question isn't seeking debugging help). – Jason C Aug 22 '16 at 10:22
  • Eeek! Can you post the code? I would really love to see why the unit tests fail. – BЈовић Aug 22 '16 at 11:35
  • Sorry, the source code is too complicate to post.@BЈовић – Yuan Wen Aug 23 '16 at 05:41

7 Answers7

7

The virtuality of the destructor will not break anything in the existing code unless there are other problems. It may even solve some (see below). However the class may not be designed as polymorphic so adding virtual to its destructor enables it as polymorphic-able which might not be desirable. Nevertheless you should be able to safely add virtuality to the destructor and it should cause no issues on itself.

Explanation

Polymorphism allows for this:

class A 
{
public:
    ~A() {}
};

class B : public A
{
    ~B() {}

    int i;
};

int main()
{
    A *a = new B;
    delete a;
}

You can take pointer to the object of type A of the class that is in fact of type B. This is useful for example for splitting interfaces (e.g. A) and implementations (e.g. B). However what will happen on delete a;?

Part of object a of type A is destroyed. But what about the part of type B? Plus that part has resources and they need to be freed. Well that is a memory leak right there. By calling delete a; you call the destructor of type A (because a is a pointer to type A), basically you call a->~a();. Destructor of type B is never called. How to solve this?

class A :
{
public:
    virtual ~A() {}
};

By adding virtual dispatch to the A's destructor (note that by declaring base destructor virtual it automatically makes all derived classes' destructors virtual even when not declared as such). Then the call to delete a; will dispatch the call of the destructor into virtual table to find the correct destructor to use (in this case of type B). That destructor will call parent destructors as usual. Neat, right?

Possible Problems

As you can see by doing this you cannot break anything per se. However there might be different issues in your design. For example there might have been a bug that "relied" on the non-virtual call of the destructor that you exposed by making it virtual, consider:

int main()
{
    B *b = new B;
    A *a = b;
    delete a;
    b->i = 10; //might work without virtual destructor, also undefined behvaiour
}

Basically object slicing, but since you did not have virtual destructor before then B part of the created object was not destroyed hence the assignment to i might work. If you made the destructor virtual then it does not exist and it will likely crash or do whatever (undefined behaviour).

Things like these might happen and in complicated code it may be hard to find. But if your destructor causes crashes after you made it virtual you likely have a bug like this somewhere in there and you had it there to begin with because as I said just making the destructor virtual cannot break anything on its own.

Yuan Wen
  • 1,343
  • 3
  • 17
  • 36
Resurrection
  • 3,462
  • 2
  • 27
  • 47
  • 2
    `delete a;` in your last example triggers UB if `A`'s destructor is not virtual. Nothing is guaranteed about the survival of `i` or anything else. – Quentin Aug 22 '16 at 08:43
  • Yes, because the program works fine. So I assume it won't implement code like Resurrection's last example.@Quentin – Yuan Wen Aug 22 '16 at 09:06
  • What else may cause the same problem?@Quentin – Yuan Wen Aug 22 '16 at 09:12
  • 1
    The question says that the base class already is polymorphic, so whatever kind of sentence "However the class may not be designed as polymorphic so adding virtual to its destructor enables it as polymorphic-able which might not be desirable." is supposed to be, it is wrong. – Ben Voigt Aug 22 '16 at 19:10
  • @BenVoigt I guess he meant that it will introduce the storage overhead of a vtable if one did not already exist, but yeah - that's unfortunately worded at best, and not relevant if the class was already being used polymorphically, which intrinsically implies it has `virtual` functions (which, as mentioned elsewhere, does not mean a `virtual` destructor is required, unless a user with a base pointer/reference can `delete` it). – underscore_d Aug 26 '16 at 16:34
6

Have a look here,

struct Component {
    int* data;

    Component() { data = new int[100]; std::cout << "data allocated\n"; }
    ~Component() { delete[] data; std::cout << "data deleted\n"; }
};

struct Base {
    virtual void f() {}
};

struct Derived : Base {
    Component c;
    void f() override {}

};

int main()
{
    Base* b = new Derived;
    delete b;
}

The output:

data allocated

but not deleted.

Conclusion

Whenever a class hierarchy has state, on the purely technical level, you want a virtual destructor all the way from the top.

It is possible that once you added that virtual destructor to your class, you triggered untested destruction logic. The sane choice here is to keep the virtual destructor you've added, and fix the logic. Otherwise, you're going to have resource and/or memory leaks in your process.

Technical Details

What's happening in the example is that, while Base has a vtable, its destructor itself isn't virtual, and that means that whenever Base::~Base() is called, it doesn't go through a vptr. In other words, it simply calls Base::Base(), and that's it.

In the main() function, a new Derived object is allocated and assigned to a variable of type Base*. When the next delete statement runs, it actually first attempts to call the destructor of the directly passed type, which is simply Base*, and then it frees the memory occupied by that object. Now, since the compiler sees that Base::~Base() isn't virtual, it doesn't attempt to go through the vptr of the object d. This means that Derived::~Derived() is never called by anyone. But since Derived::~Derived() is where the compiler generated the destruction of the Component Derived::c, that component is never destroyed either. Therefore, we never see data deleted printed.

If Base::~Base() were virtual, what would happen is that the delete d statement would go through the vptr of the object d, calling the destructor, Derived::~Derived(). That destructor, by definition, would first call Base::~Base() (this is auto-generated by the compiler), and then destroy its internal state, that is, Component c. Thus, the entire destruction process would have completed as expected.

Yam Marcovic
  • 7,467
  • 25
  • 37
  • What kind of untested destruction logic? More details? @Yam Marcovic – Yuan Wen Aug 22 '16 at 08:52
  • @YuanWen: In current example `~Derived` and so `~Component`. – Jarod42 Aug 22 '16 at 08:54
  • 4
    It's possible, for example, that one of your derived classes has a member, something like `Component` in my example, whose destruction logic is faulty in particular contexts, such as your failed test. Now, since previously it wasn't even called, you passed the test, but you probably leaked resources and/or memory. – Yam Marcovic Aug 22 '16 at 08:55
  • You mean in `~Component()`, it may be `delete data ` instead of `delete [] data`? But if `~Component()` not called, the result could be worse because nothing has been freed instead of some. @YamMarcovic – Yuan Wen Aug 22 '16 at 09:22
  • No, `~Component()` is written just fine here. The problem is that without the virtual destructor in `Base`, it simply doesn't get called when you're calling `delete` on a `Base*`. – Yam Marcovic Aug 22 '16 at 09:24
  • I know, but in my case, base class with virtual destructor failed, while non-virtual destructor succeed.@YamMarcovic – Yuan Wen Aug 22 '16 at 09:33
  • 4
    @YuanWen: which probably means that someone is relying on the fact that some data member is leaked, or some destruction logic of a derived class is faulty (and the program runs fine because erroneously it isn't invoked). – Matteo Italia Aug 22 '16 at 09:37
  • Once again, C++ does not define the behavior here as "calls the base destructor but not the derived one". It is undefined behavior. – Ben Voigt Aug 26 '16 at 17:20
6

It depends on what your code is doing, obviously.

In general terms, making the destructor of the base class virtual is only necessary if you have a usage like

 Base *base = new SomeDerived;
    // whatever
 delete base;

Having a non-virtual destructor in Base causes the above to exhibit undefined behaviour. Making the destructor virtual eliminates the undefined behaviour.

However, if you do something like

{   // start of some block scope

     Derived derived;

      //  whatever

}

then it is not necessary for the destructor to be virtual, since the behaviour is well defined (the destructors of Derived and its bases are invoked in reverse order of their constructors).

If changing the destructor from non-virtual to virtual causes a test case to fail, then you need to examine the test case to understand why. One possibility is that the test case relies on on some specific incantation of undefined behaviour - which means the test case is flawed, and may not succeed in different circumstances (e.g. building the program with a different compiler). Without seeing the test case (or an MCVE that is representative of it), however, I'd hesitate to claim it DOES rely on undefined behaviour

Peter
  • 32,539
  • 3
  • 27
  • 63
4

It may break some tests if someone who derived from a base class changed the ownership policy of class's resources:

struct A 
{ 
    int * data; // does not take ownership of data
    A(int* d) : data(d) {}
     ~A() { }
};

struct B : public A // takes ownership of data
{
    B(int * d) : A (d) {}
    ~B() { delete data; }
};

And usage:

int * t = new int(8);
{
    A* a = new B(t);
    delete a;
}
cout << *t << endl;

Here making the A's destructor virtual will cause UB. I don't think that such usage can be called a good practice, though.

Month
  • 233
  • 1
  • 9
  • How so? When A's destructor is called, the pointer t remains valid. – Month Aug 23 '16 at 13:14
  • 1
    "When A's destructor is called" is a strawman. According to the Standard, there is nothing in your code that calls A's destructor. You seem to think that polymorphic delete when the destructor is non-virtual will abandon the derived class and correctly destroy the base subobject, but what the Standard actually says is very different. – Ben Voigt Aug 23 '16 at 14:25
  • "In the first alternative (delete object), if the static type of the object to be deleted is different from its dynamic type, the static type shall be a base class of the dynamic type of the object to be deleted and **the static type shall have a virtual destructor or the behavior is undefined**" – Ben Voigt Aug 23 '16 at 14:25
  • Thanks for clarification, I really didn't know that. – Month Aug 23 '16 at 19:37
3

You may "safely" add virtual to the destructor.

You may fix Undefined Behavior (UB) if equivalent of delete base is called, and then call the right destructors. If the subclass destructor is buggy, then you change UB by an other bug.

Jarod42
  • 173,454
  • 13
  • 146
  • 250
  • UB is short for undefined behavior?@Jarod42 – Yuan Wen Aug 22 '16 at 07:54
  • 1
    Maybe explain when UB could happen? – juanchopanza Aug 22 '16 at 07:55
  • The original base class works fine. But changing destructor from non-virtual to virtual causes bugs. How to fix it if I still want the destructor to be virtual?@juanchopanza – Yuan Wen Aug 22 '16 at 08:02
  • 7
    @YuanWen If the behaviour changes, that is indicative that the destructor should have been virtual. So you need to fix the other bugs in your code. – juanchopanza Aug 22 '16 at 08:04
  • 2
    Yeah, if making a destructor virtual causes bugs, something seems pretty fishy... – qxz Aug 22 '16 at 08:05
  • 2
    Maybe whoever wrote it to begin with discovered that if they sneakily made that destructor not virtual, they could pretend they fixed their original bugs. In which case: have fun! – Jason C Aug 22 '16 at 08:48
3

One thing that could change is the class's layout type. Adding a virtual destructor can change a class from being a standard-layout type to a non-standard-layout class. So anything were you rely on the class being a POD or standard-layout type, e.g.

  • memcpy or memmve classes around
  • pass to C functions

will be UB.

Jens
  • 8,115
  • 1
  • 21
  • 37
3

I know of exactly one case where a type is

  • used as a base class
  • has virtual functions
  • the destructor is not virtual
  • making the destructor virtual breaks things

and that is when the object conforms to an external ABI. All COM interfaces on Windows meet all four criteria. This isn't undefined behavior, but non-portable implementation-specific guarantees concerning the virtual dispatch machinery.

Regardless of your OS, it boils down to "The One-Definition Rule". You can't modify a type unless you recompile every piece of code using it against the new definition.

Ben Voigt
  • 260,885
  • 36
  • 380
  • 671
  • If he deletes his derived class, then if that class itself releases another object *O* in its destructor, and *O*'s destruction logic fails in some way for some reason, then the test will break by making the base destructor virtual. However, the proper fix is clearly to fix the destruction logic, and not keep the destructor non-virtual. – Yam Marcovic Aug 27 '16 at 18:46
  • @Yam: Have you ever implemented IUnknown for a COM object? Or any design pattern where objects free themselves through a virtual member function which is not the destructor? When an interface specification defines that a type has a non-virtual destructor, you must not add the `virtual` keyword, since doing so would break the One Definition Rule. – Ben Voigt Aug 28 '16 at 06:10