Pure virtual method called

Question

I understand why calling a virtual function from a constructor is bad, but I'm not sure why defining a destructor would result in a "pure virtual method called" exception. The code uses const values to reduce the use of dynamic allocation - possibly also the culprit.

#include <iostream>
using namespace std;

class ActionBase {
 public:
    ~ActionBase() { } // Comment out and works as expected

    virtual void invoke() const = 0;
};

template <class T>
class Action : public ActionBase {
 public:
    Action( T& target, void (T::*action)())
     : _target( target ), _action( action ) { }

    virtual void invoke() const {
        if (_action) (_target.*_action)();
    }

    T&   _target;
    void (T::*_action)();
};

class View {
 public:
    void foo() { cout << "here" << endl; }
};

class Button : public View {
 public:
    Button( const ActionBase& action )
     : _action( action ) { }

    virtual void mouseDown() {
        _action.invoke();
    }

 private:
    const ActionBase& _action;
};

int main( int argc, char* argv[] )
{
    View view;
    Button button = Button( Action<View>( view, &View::foo ) );
    button.mouseDown();

    return 0;
}

Phosphorate

You should make ActionBase's destructor virtual. See parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7 – Presidentelect

Add a destructor to Action<> that prints out when it is destroyed and you will see it dying before it can be used and thus invoking undefined behavior. – Special

Answer 1

You have Undefined Behavior. As the parameter to Button's ctor is a const& from a temporary, it is destroyed at the end of that line, right after the ctor finishes. You later use _action, after Action's dtor has already run. Since this is UB, the implementation is allowed to let anything happen, and apparently your implementation happens to do something slightly different depending on whether you have a trivial dtor in ActionBase or not. You get the "pure virtual called" message because the implementation is providing behavior for calling ActionBase::invoke directly, which is what happens when the implementation changes the object's vtable pointer in Action's dtor.

I recommend using boost.function or a similar 'action callback' library (boost has signals and signals2, for example).

Emporium

About undefined-ness... I checked the assembly, it seems that gcc didn't destroy Action hierarchy at all, if destructor was "trivial"--i.e. compiler-generated. So the code correctly caught vtable pointer without destructor since it remained in memory unoverwritten. – Sangraal

There's no correctly catching this; it's undefined behavior and all bets are off in the entire program (as far as the standard guarantees). What you found does make sense though, in the trivial-dtor case the vtable pointer isn't updated, so no error message. – Emporium

Demons flying from the nose and all that. – Solenne

Thanks Roger, now I understand what's going on. I only recently started using const for non-trivial code, so the scope of action wasn't entirely clear to me. – Phosphorate

Answer 2

Set a breakpoint on the destructor and it will become clear what is happening. Yup, you are passing a temporary instance of Action<> to the Button constructor. It is destroyed after the button construct runs. Write it like this and the problem disappears:

View view;
Action<View> event(view, &View::foo);
Button button = Button( event ); 
button.mouseDown();

Well, that's not a practical solution, event is not going to be in scope for a real mouseDown invocation. The Button constructor is going to have to create a copy of the "event" argument or it is going to have to manage a pointer to the delegate.

Saprogenic

+1. But why 'Action<View> event = Action<View>(view,&view::foo)'? Why not 'Action<View> event(view,&View::foo)'; – Special

Answer 3

A class with virtual functions should always have a virtual destructor, so ~ActionBase() should be virtual, (and so should ~Action()). If you turn on more compiler warning you will get a warning about this.

Essentially, because of the lookup rules, the destructor is called for a type that the compiler knows cannot be instantiated (pure virtual), so it knows something must have gone wrong.

I'm sure someone else can explain better than I can :)

Escape

While you're very right in your advice, that doesn't have anything to do with the problem in subject. – Sangraal

Thanks for the quick answer guys. With a virtual destructor, this still compiles and executes with a "pure virtual method called" error with gcc-ubuntu-64 4.3.3. The first produces the error, the next does not. What is inherently different between the two? The second takes the address of a temporary, which of course is not good. Does the first also? Button button = Button( Action<View>( view, &View::foo ) ); Action<View> action( view, &View::foo ); Button button2 = Button( action ); – Phosphorate

Mike: The issue isn't taking the taking the address of a temporary, but using a reference after the lifetime has ended for the object to which it refers (see my answer). – Emporium