51

Is this safe ?

class Derived:  public PublicBase, private PrivateBase
{
 ... 

   ~Derived()
   {
      FunctionCall();
   }

   virtual void FunctionCall()
   {
      PrivateBase::FunctionCall();
   }
}

class PublicBase
{
   virtual ~PublicBase(){};
   virtual void FunctionCall() = 0;
}

class PrivateBase
{
   virtual ~PrivateBase(){};
   virtual void FunctionCall()
   {
    ....
   }
}


PublicBase* ptrBase = new Derived();
delete ptrBase;

This code crases sometimes with IP in a bad address.

That is not a good idea to call a virtual function on constructor is clear for everyone.

From articles like http://www.artima.com/cppsource/nevercall.html I understand that destructor is also a not so good place to call a virtual function.

My question is "Is this true ?" I have tested with VS2010 and VS2005 and PrivateBase::FunctionCall is called. Is undefined behavior ?

cprogrammer
  • 5,503
  • 3
  • 36
  • 56
  • 4
    You get undefined behaviour from deleting a pointer-to-base where the destructor isn't marked `virtual`. Also, you should have an ambiguity between the two bases since both functions have the same signature - which one are you overriding? IOW, post your real code, this one doesn't even compile. – Xeo Aug 23 '12 at 13:42
  • Sorry for confusion: the real code is too complex, Derived does not have a virtual destructor, The Base classes do have. – cprogrammer Aug 23 '12 at 13:51
  • 3
    @cprogrammer: Once a class has a virtual destructor, all objects that inherit from it *have* virtual destructors, whether the code shows it or not. I personally prefer explicitly typing the `virtual` keyword, but it is fully optional (The same goes for any other virtual function: overrides of the virtual function will be virtual, whether declared as such or not in the code). – David Rodríguez - dribeas Aug 23 '12 at 14:02
  • Try overriding `FunctionCall` in a `TooDerived : pulic Derived` class. That won't be called. – zahir Oct 02 '15 at 15:56
  • **−1** This code won't compile due to missing semicolons. That's not important in itself but it means that this is **not the real code**. Also, the example is **incomplete**, and on that basis I'm voting to close. – Cheers and hth. - Alf Aug 14 '16 at 10:27
  • 3
    @Cheersandhth.-Alf - That seems overzealous. "Real code" is not the underlying objective; the objective is "code that adequately/unambiguously illustrates the crux of the problem" - which this code clearly does. – Oliver Charlesworth Aug 14 '16 at 10:38
  • 1
    a good reference: https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP50-CPP.+Do+not+invoke+virtual+functions+from+constructors+or+destructors – Chen Li Dec 03 '17 at 04:45
  • @Chen Li :: The link is very useful. It also shows a general solution too. Thank. – cppBeginner Jun 08 '22 at 04:45

5 Answers5

78

I am going to go against the flow here... but first, I must assume that your PublicBase destructor is virtual, as otherwise the Derived destructor will never be called.

It is usually not a good idea to call a virtual function from a constructor/destructor

The reason for this is that dynamic dispatch is strange during these two operations. The actual type of the object changes during construction and it changes again during destruction. When a destructor is being executed, the object is of exactly that type, and never a type derived from it. Dynamic dispatch is in effect at all time, but the final overrider of the virtual function will change depending where in the hierarchy you are.

That is, you should never expect a call to a virtual function in a constructor/destructor to be executed in any type that derived from the type of the constructor/destructor being executed.

But

In your particular case, the final overrider (at least for this part of the hierarchy) is above your level. Moreover, you are not using dynamic dispatch at all. The call PrivateBase::FunctionCall(); is statically resolved, and effectively equivalent to a call to any non-virtual function. The fact that the function is virtual or not does not affect this call.

So yes it is fine doing as you are doing, although you will be forced to explain this in code reviews as most people learn the mantra of the rule rather than the reason for it.

Nawaz
  • 353,942
  • 115
  • 666
  • 851
David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
  • Calling delete will call `Derived::~Derived()` (because we have virtual destructor) that will call `FunctionCall` (during destruction is that type: PublicBase) so will resolve to `PublicBase::FunctionCall()` that is pure virtual. Is that corect ? – cprogrammer Aug 23 '12 at 14:02
  • 1
    @cprogrammer: Until the closing brace of `~Derived`, the type of the object is `Derived` (and by extension also `PublicBase` and `PrivateBase`) This is irrespective of the static type of the pointer through which you are deleting (granted that the destructor of that type is virtual, as is the case). During the execution of `~Derived`, the call to `PrivateBase::FunctionCall()` is **not** dynamic dispatch, it is a direct call to the function (due to the qualification `PrivateBase::`) – David Rodríguez - dribeas Aug 23 '12 at 14:07
  • So, the call to `FunctionCall` in `Derived::~Derived()` is resolved to `Derived::FunctionCall` or `PublicBase::FunctionCall` (the object is of exactly that type) ? I understand that `PrivateBase::FunctionCall()` is not a dynamic dispatch due to the qualification, but that call is at the next step. – cprogrammer Aug 23 '12 at 14:15
  • @cprogrammer: I am a bit confused with your question. When `delete` is called, `~Derived` will be called through dynamic dispatch, as `~PublicBase` is virtual. At this point the object is of type `Derived` and it is at this point that the destructor body is executed and `PrivateBase::FunctionCall` is called. – David Rodríguez - dribeas Aug 23 '12 at 14:33
  • @DavidRodríguez-dribeas It *is* dynamic despatch, but via the VFT of the class executing the current constructor, not the VFT of `this`. – user207421 Aug 14 '16 at 11:02
  • @EJP: `PrivateBase::FunctionCall()` is static dispatch, qualified function calls never use dynamic dispatch. – David Rodríguez - dribeas Sep 06 '16 at 11:15
30

Is this safe ?

Yes. Calling a virtual function from a constructor or destructor dispatches the function as if the object's dynamic type were that currently being constructed or destroyed. In this case, it's called from the destructor of Derived, so it's dispatched to Derived::FunctionCall (which, in your case, calls PrivateBase::FunctionCall non-virtually). All of this is well defined.

It's "not a good idea" to call virtual functions from a constructor or destructor for three reasons:

  • It will cause unexpected behaviour if you call it from a base class and (erroneously) expect it to be dispatched to an override in a derived class;
  • It will cause undefined behaviour if it is pure virtual;
  • You'll keep having to explain your decision to people who believe that it's always wrong to that.
Mike Seymour
  • 249,747
  • 28
  • 448
  • 644
3

In general, it is not a good idea to call a virtual function, unless the object of the class it might get dispatched to (i.e., the "full" object of the most-derived class) is fully-constructed. And this is not the case

  • until all the constructors finish execution
  • after any destructor finishes execution
Grzegorz Herman
  • 1,875
  • 11
  • 22
  • There are virtual destructors in the code, so the first destructor called is Derived that calls a base function. Second condition is meet ? – cprogrammer Aug 23 '12 at 13:47
  • 1
    If the code was correctly written (virtual dtors, unambiguous overriding of baseclass methods), the bullets wouldn't apply and the code would actually be one of the rare cases that's "fine". The problems stem from calling virtual functions from the *base class ctor / dtor*. – Xeo Aug 23 '12 at 13:49
  • 2
    No, the "full" object isn't involved in constructor and destructor semantics. Virtual function calls from constructors and destructors go to the version of the function for the type whose constructor or destructor is being run, not necessarily the most derived type. – Pete Becker Aug 23 '12 at 14:23
  • @PeteBecker: you are right -- the type of the object (including its vtable) should change during construction/destruction, as pointed out by David. I remember having issues with that some time ago though -- but maybe the old compiler was broken, both modern gcc and clang get it right (I just checked to be sure). – Grzegorz Herman Aug 23 '12 at 14:37
1

It is a very bad idea according to scott: link

This is what i have compiled and run to help myself gain a better understanding of the destruction process, you might also find it helpful

#include <iostream>
using namespace std;


class A {
public:
  virtual void method() {
    cout << "A::method" << endl;
  }

  void otherMethod() {
    method();
  }

  virtual ~A() {
    cout << "A::destructor" << endl;
    otherMethod();
  }

};

class B : public A {
public:
  virtual void method() {
    cout << "B::method" << endl;
  }

  virtual ~B() {
    cout << "B::destructor" << endl;
  }
};

int main() {

  A* a = new B();

  a->method();

  delete a;

}
sji
  • 1,877
  • 1
  • 13
  • 28
  • 1
    No, data members are not destroyed until after the body of the destructor. There's no danger in accessing those members from the destructor, even through a function call, and even through a virtual call. – Pete Becker Aug 23 '12 at 14:20
  • @PeteBecker I was under the impression that the body of `~Derived` was executed before the call to `~Base`. – sji Aug 23 '12 at 14:25
  • 2
    that's right. The body of `~Derived` executes first, then data members of `Derived` are destroyed, then `~Base` is executed. – Pete Becker Aug 23 '12 at 14:28
  • @PeteBecker soooo .... if `Base` calls `Derived.Somthing()` then we have big problems since `Derived` has already called `delete important_pointer` surely – sji Aug 23 '12 at 14:32
  • 1
    if `~Base()` calls a virtual function the call goes to the version defined for `~Base()`. It does not call derived versions. – Pete Becker Aug 23 '12 at 14:34
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/15724/discussion-between-sji-and-pete-becker) – sji Aug 23 '12 at 14:48
0

Is this safe ?

Yes and No.

Yes, because your example as-is is well defined and will work. All of that is well explained by other answers. Also, this code is totally safe because it won't compiler the way it's written: private dtors in base classes etc.

The reason it's not safe to do the way your example does is because this code assumes that nobody else will override FunctionCall from your Derived class and won't expect that override to be called on object destruction. Most likely compilers will complain about this. You can improve your code by marking your FunctionCall as final:

class Derived : public PublicBase, private PrivateBase
{
 ... 
   virtual void FunctionCall() final;
}

or your Derived class as final:

class Derived final : public PublicBase, private PrivateBase
{
 ... 
   virtual void FunctionCall();
}

If you compile on older compilers or cannot use c++11 for any other reason, then you may at least be more explicit here and write in your code exactly what will happen at runtime regardless if FunctionCall is overridden by any descendant of your Derived class:

class Derived : public PublicBase, private PrivateBase
{
 ... 
   ~Derived()
   {
      Derived::FunctionCall(); // be explicit which FunctionCall will be called
   }

   virtual void FunctionCall()
   {
      PrivateBase::FunctionCall();
   }
}
Pavel P
  • 15,789
  • 11
  • 79
  • 128