views:

425

answers:

3

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;
}
+2  A: 

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 :)

Autopulated
While you're very right in your advice, that doesn't have anything to do with the problem in subject.
Pavel Shved
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, Action<View> action( view, Button button2 = Button( action );
Mike Austin
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).
Roger Pate
+5  A: 

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

Roger Pate
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.
Pavel Shved
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.
Roger Pate
Demons flying from the nose and all that.
greyfade
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.
Mike Austin
+2  A: 

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.

Hans Passant
+1. But why 'Action<View> event = Action<View>(view,
Martin York
C# habit, fixed. Thanks.
Hans Passant