If I change the destructor of one base class from non-virtual to virtual, what will happen?

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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

Answer 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.