0

The following "Event" code snippet shows the "pure virtual function call" error. However, as mentioned in the title, it happens only when deploying on DEBUG. What makes me curious is why it works flawlessly on RELEASE and why it does even crash (on DEBUG). Alternatively, you can see the snippet here.

#include <list>
#include <iostream>
#include <algorithm>

// use base class to resolve the problem of how to put into collection objects of different types
template <typename TPropertyType>
struct PropertyChangedDelegateBase
{
    virtual ~PropertyChangedDelegateBase(){};
    virtual void operator()(const TPropertyType& t) = 0;
};

template <typename THandlerOwner, typename TPropertyType>
struct PropertyChangedDelegate : public PropertyChangedDelegateBase<TPropertyType>
{
    THandlerOwner* pHandlerOwner_;

    typedef void (THandlerOwner::*TPropertyChangeHandler)(const TPropertyType&);
    TPropertyChangeHandler handler_;

public:
    PropertyChangedDelegate(THandlerOwner* pHandlerOwner, TPropertyChangeHandler handler) :
      pHandlerOwner_(pHandlerOwner), handler_(handler){}

    void operator()(const TPropertyType& t)
    {
        (pHandlerOwner_->*handler_)(t);
    }
};

template<typename TPropertyType>
class PropertyChangedEvent
{
public:
    virtual ~PropertyChangedEvent(){};

    void add(PropertyChangedDelegateBase<TPropertyType>* const d)
    {
        std::list<PropertyChangedDelegateBase<TPropertyType>* const>::const_iterator it = std::find(observers_.begin(), observers_.end(), d);
        if(it != observers_.end())
            throw std::runtime_error("Observer already registered");

        observers_.push_back(d);
    }


    void remove(PropertyChangedDelegateBase<TPropertyType>* const d)
    {      
        std::list<PropertyChangedDelegateBase<TPropertyType>* const>::const_iterator it = std::find(observers_.begin(), observers_.end(), d);
        if(it != observers_.end())
            observers_.remove(d);
    }  

    // notify
    void operator()(const TPropertyType& newValue)
    {
        std::list<PropertyChangedDelegateBase<TPropertyType>* const>::const_iterator it = observers_.begin();
        for(; it != observers_.end(); ++it)
        {
            (*it)->operator()(newValue);
        }
    }

protected:
    std::list<PropertyChangedDelegateBase<TPropertyType>* const> observers_;
};

class PropertyOwner
{
    int property1_;
    float property2_;

public:
    PropertyChangedEvent<int> property1ChangedEvent;
    PropertyChangedEvent<float> property2ChangedEvent;

    PropertyOwner() :
        property1_(0),
        property2_(0.0f)
    {}  

    int property1() const {return property1_;}
    void property1(int n)
    {
        if(property1_ != n)
        {
            property1_ = n;
            property1ChangedEvent(n);
        }
    }

    float property2() const {return property2_;}
    void property2(float n)
    {
        if(property2_ != n)
        {
            property2_ = n;
            property2ChangedEvent(n);
        }
    }
};

struct PropertyObserver
{  
    void OnPropertyChanged(const int& newValue)
    {
        std::cout << "PropertyObserver::OnPropertyChanged() -> new value is: " << newValue << std::endl;
    }
};

int _tmain(int argc, _TCHAR* argv[])
{
    PropertyOwner propertyOwner;
    PropertyObserver propertyObserver;

    // register observers
    PropertyChangedDelegate<PropertyObserver, int> delegate(&propertyObserver, &PropertyObserver::OnPropertyChanged);

    propertyOwner.property1ChangedEvent.add(&delegate); // Ok!
    propertyOwner.property1ChangedEvent.add(&PropertyChangedDelegate<PropertyObserver, int>(&propertyObserver, &PropertyObserver::OnPropertyChanged)); // Error: Virtual pure function call (Debug only)
    propertyOwner.property1(1);

    return getchar();
}
Yves Calaci
  • 1,019
  • 1
  • 11
  • 37

3 Answers3

0

I would assume that the error is misnomer and that the problem is more likely to do with the scope that the second delegate lives. Plus declaring it outside is easier to read.

Passing around an object created on the stack rather than the heap by reference is usually a bad idea. Once the item declaration is out of scope the object is usually forgotten about.

Edward Addley
  • 36
  • 4
  • What do you mean "declaring it outside is easier to read"? Also, can you elighten me about "passing around an object created on the stack rather than the heap by reference is usually a bad idea. Once the item declaration is out of scope the object is usually forgotten about"? I'm kinda lost. – Yves Calaci Mar 02 '16 at 11:26
  • Declaring your delegate before passing it into the method that registers it as an observer is easier to read. This is subjective though but given something a name is more helpful than not. You want to look more in-depth into memory management particularly "stack vs heap" – Edward Addley Mar 02 '16 at 13:45
  • The example shows both declaring outside and "inside", though I wanted to make sure whats going on. Despite that, I am now managing the observers internally in the Event class. I found this approach much better because those classes are supposed to be wrapped in a lib (I'm developing a lib that parses TMX files - see mapeditor.org) and it wouldnt look very handy to make the users have a bunch of "outside" delegates in their classes. Here's what I have done: [link](http://pastebin.com/pdYkzPAz) What do you think? – Yves Calaci Mar 02 '16 at 14:47
  • I like the flexibility of it and it's a nice use of templates. Does it need to be that flexible though? It would be simpler to have a class with virtual methods for each event and inherit it. I've been doing too much iOS recently and their delegate pattern is very easy to depend upon. – Edward Addley Mar 02 '16 at 16:16
  • Thanks! Well, honestly, I dont know. The reason is: this does offer a big flexibility and this shall be considered, since its a library. I, however, dont like the usage of annonymous types (templates) in C++ due to its implementation (organizing templated headers). The previous approach is exactly what you said: instead of having a couple Events in the AbstractParser, it inherited from ParseSubject and whichever class that wanted to listen to these events should inherit from ParseObserver. ParseObserver is nothing, but a class with a bunch of void functions such as onParsed(X), onParsed(y)... – Yves Calaci Mar 02 '16 at 17:11
  • Continuing... These onParsed functions are private and is shared with ParseSubject because its a friend of ParseObserver. Using friendship allows the users not to have public event handlers in their classes, which is great. The Events approach just sounds better to me, although I need a way to organize that templated functions implementation (inline/export/etc). – Yves Calaci Mar 02 '16 at 17:15
0

The general issue is that you are binding to a temporary that gets destroyed and thus has an empty vtable and of course it generates a pure virtual call when invoked on the change of the property. If you add a dtor for the base class this is quite easy to observe:

#include <list>
#include <iostream>
#include <algorithm>

// use base class to resolve the problem of how to put into collection objects of different types
template <typename TPropertyType>
struct PropertyChangedDelegateBase
{
    virtual ~PropertyChangedDelegateBase(){};
    virtual void operator()(const TPropertyType& t) = 0;
};

template <typename THandlerOwner, typename TPropertyType>
struct PropertyChangedDelegate : public PropertyChangedDelegateBase<TPropertyType>
{
    THandlerOwner* pHandlerOwner_;

    typedef void (THandlerOwner::*TPropertyChangeHandler)(const TPropertyType&);
    TPropertyChangeHandler handler_;

public:
    PropertyChangedDelegate(THandlerOwner* pHandlerOwner, TPropertyChangeHandler handler) :
        pHandlerOwner_(pHandlerOwner), handler_(handler)
    {
        std::cout << "0x" << std::hex << this << " created!" << std::endl;
    }

    void operator()(const TPropertyType& t)
    {
        (pHandlerOwner_->*handler_)(t);
    }

    ~PropertyChangedDelegate()
    {
        std::cout << "0x" << std::hex << this << " destroyed!" << std::endl;
    }
};

template<typename TPropertyType>
class PropertyChangedEvent
{
public:
    virtual ~PropertyChangedEvent(){};

    void add(PropertyChangedDelegateBase<TPropertyType>* const d)
    {
        std::list<PropertyChangedDelegateBase<TPropertyType>* const>::const_iterator it = std::find(observers_.begin(), observers_.end(), d);
        if (it != observers_.end())
            throw std::runtime_error("Observer already registered");

        observers_.push_back(d);
    }


    void remove(PropertyChangedDelegateBase<TPropertyType>* const d)
    {
        std::list<PropertyChangedDelegateBase<TPropertyType>* const>::const_iterator it = std::find(observers_.begin(), observers_.end(), d);
        if (it != observers_.end())
            observers_.remove(d);
    }

    // notify
    void operator()(const TPropertyType& newValue)
    {
        std::list<PropertyChangedDelegateBase<TPropertyType>* const>::const_iterator it = observers_.begin();
        for (; it != observers_.end(); ++it)
        {
            std::cout << "Invoking 0x" << std::hex << *it << std::endl;
            (*it)->operator()(newValue);
        }
    }

protected:
    std::list<PropertyChangedDelegateBase<TPropertyType>* const> observers_;
};

class PropertyOwner
{
    int property1_;
    float property2_;

public:
    PropertyChangedEvent<int> property1ChangedEvent;
    PropertyChangedEvent<float> property2ChangedEvent;

    PropertyOwner() :
        property1_(0),
        property2_(0.0f)
    {}

    int property1() const { return property1_; }
    void property1(int n)
    {
        if (property1_ != n)
        {
            property1_ = n;
            property1ChangedEvent(n);
        }
    }

    float property2() const { return property2_; }
    void property2(float n)
    {
        if (property2_ != n)
        {
            property2_ = n;
            property2ChangedEvent(n);
        }
    }
};

struct PropertyObserver
{
    void OnPropertyChanged(const int& newValue)
    {
        std::cout << "PropertyObserver::OnPropertyChanged() -> new value is: " << newValue << std::endl;
    }
};

int main(int argc, char* argv[])
{
    PropertyOwner propertyOwner;
    PropertyObserver propertyObserver;

    // register observers
    PropertyChangedDelegate<PropertyObserver, int> delegate(&propertyObserver, &PropertyObserver::OnPropertyChanged);

    propertyOwner.property1ChangedEvent.add(&delegate); // Ok!
    propertyOwner.property1ChangedEvent.add(&PropertyChangedDelegate<PropertyObserver, int>(&propertyObserver, &PropertyObserver::OnPropertyChanged)); // Error: Virtual pure function call (Debug only)
    propertyOwner.property1(1);

    return getchar();
}

Crashy crashy

Basically you are just running into undefined behavior - the object is destroyed in both cases, but in Release the vtable is not destroyed so you get by.

Community
  • 1
  • 1
Rudolfs Bundulis
  • 11,636
  • 6
  • 33
  • 71
0

This:

propertyOwner.property1ChangedEvent.add(
    &PropertyChangedDelegate<PropertyObserver, int>(
    &propertyObserver, 
    &PropertyObserver::OnPropertyChanged)
);

You are capturing a pointer to a temporary object PropertyChangedDelegate<PropertyObserver, int>. Pointer to this object becomes invalid as soon as function call is over and temporary is destroyed. Dereferencing this pointer is undefined behavior.

In your program, memory ownership relations are critical and you should think them through carefully. You need to ensure that all your pointers outlive objects that rely on them, either manually:

PropertyChangedDelegate<PropertyObserver, int> delegate2 = {
    &propertyObserver, 
    &PropertyObserver::OnPropertyChanged
};

propertyOwner.property1ChangedEvent.add(&delegate2);

or by using smart pointers (std::unique_ptr<>, std::shared_ptr<>).

Another bug:

C++11 compliant compier should not allow you doing this:

std::list<PropertyChangedDelegateBase<TPropertyType>* const> observers_;

The error I got with Visual Studio 2015 is:

The C++ Standard forbids containers of const elements because allocator is ill-formed.`

See: Does C++11 allow vector<const T>?

Bonus:

Your C++ style looks quite a bit obsolete. You might want to try automatic type deduction:

for(auto it = observers_.begin(); it != observers_.end(); ++it)
{
    (*it)->operator()(newValue);
}

or, better, ranged for loops:

for(auto observer : observers)
{
    observer(newValue);
}

You might want to take a look to:

Community
  • 1
  • 1
Ivan Aksamentov - Drop
  • 12,860
  • 3
  • 34
  • 61
  • So does that object "lives" for as long as the ".add(&...)" function is being executed? Thats what I thought but, that object is pushed into a vector. Shouldnt it live for as long as the ".add(&...)" function's class lives (PropertyChangedEvent)? I'm aware about the style youre talking about, I've fixed it in my current implementation. That example was copied from here: [link](http://stackoverflow.com/a/10435059/5446775). – Yves Calaci Mar 02 '16 at 12:10
  • @YvesHenri Error is happening in `propertyOwner.property1(1);`, not the line you've commented. You push pointers (an integer address of the value) in your container. Acquiring a raw pointer does not prevent objects from being destructed. Also there is another bug with `const` pointers (I added it to the answer) – Ivan Aksamentov - Drop Mar 02 '16 at 17:34