2

In this problem, assume that we have handled all pointers in a nice, careful manner - to prevent question bloat I don't want to include my cleanup code here!

Let's say we have two classes, Foo and Bar with class definitions as follows:

class Foo
{
    public:
        Foo();
        void fooFn();
};

class Bar
{
    public:
        Bar();
        void barFn();
};

Assume that it is necessary that Foo and Bar have no inheritance relationship, but we need to call both fooFn and barFn in response to some stimulus. We can create a controller class with a container from which to call fooFn and barFn on specific instances of Foo and Bar - a static std::vector for example - but we run into an issue: pointers to member functions of Foo and Bar instances are of different types.

By using a static vector< std::function<void()>* > in the controller class, we can make a workaround. Foo and Bar instances can have a function which adds pointers to the vector through a lambda function which captures this:

void Foo::registerFnPointer()
{
    ControllerClass::function_vector.push_back( new [this](){ return this->fooFn(); } );
}

I have tested this method, and it appears to work without any problems. That said, I am concerned about the issues that could be caused by circumventing the type difference mentioned before... Am I worrying over nothing? Is there a better way to accomplish equivalent functionality?

Conduit
  • 2,675
  • 1
  • 26
  • 39
  • An [adapter class](http://codereview.stackexchange.com/questions/57173/interface-based-polymorphic-collection) could help. – jliv902 Sep 12 '14 at 19:22
  • @jliv902 I'm not very familiar with software design patterns - I'll give that a look. Thanks! – Conduit Sep 12 '14 at 19:32
  • 2
    These are not [forward declarations](http://stackoverflow.com/questions/4757565/c-forward-declaration). – nwp Sep 12 '14 at 19:33
  • 5
    Why `vector*>` instead of simply `vector>` and `ControllerClass::function_vector.push_back([this]{ return fooFn(); });`? – Casey Sep 12 '14 at 19:33
  • @nwp Likely my mistake. What do you call the declaration of a class and its members where no member is defined (e.g. the stuff in a header file)? – Conduit Sep 12 '14 at 19:35
  • @Casey I gave that a shot in my code, but got a compiler error... Perhaps I just did something dumb - I'll give it another try when I have a chance. If I CAN do it that way, it would make cleanup less of a chore :) – Conduit Sep 12 '14 at 19:36
  • @Casey actually, I just remembered: in my actual application, it was more useful to store the functions in a `std::set` - the pointer implies an order to get around the fact that `std::function`s have no < operator. – Conduit Sep 12 '14 at 19:39
  • 1
    @Conduit You call that a class declaration. – cdhowie Sep 12 '14 at 19:40
  • 2
    A forward declaration would be `class Foo;`. I guess you could say you declare the class with forward declaring the member functions, but for functions it is more common to say declaration and definition. – nwp Sep 12 '14 at 19:41
  • @cdhowie,@nwp Gotcha - those of us learning from internet resources don't pick up on terminology as well, IMO. – Conduit Sep 12 '14 at 19:42
  • 2
    You could also just use `std::bind`. [**see it live**](http://ideone.com/bL1UHO). – WhozCraig Sep 12 '14 at 19:51
  • 1
    If you need `operator <` for `std::function` than you should add it instead of using pointers. `bool operator < (std::function &lhs, std::function &rhs){ return false; }` would do the trick. But then you basically lose the advantages of a `set`. If you do not require a `set` consider switching to `vector`. A better comparison function might be `return lhs.target() < rhs.target();`, but without knowing what you are trying to achieve it is hard to tell. – nwp Sep 12 '14 at 19:52
  • I've always been a little wary of tacking things onto the standard libraries... That said: using the `target` function would probably do the trick safely. Have to add that to my playbook! – Conduit Sep 12 '14 at 19:56
  • @WhozCraig wow, that's so much cleaner, and the use of emplace_back is pretty slick, too... Definitely going to try that out. – Conduit Sep 12 '14 at 20:25
  • @WhozCraig just finished testing the `std::bind` version you mentioned - works great and 100x more readable than lambdas, thank you! – Conduit Sep 14 '14 at 16:14

2 Answers2

1

The only problem I see has actually nothing to do with the functors but has to do with object lifetime. That is: I'm not sure how you ensure that you always de-register the functors registered with ControllerClass whenever an Foo or Bar instance gets destroyed.

You mention however that you do proper memory management.

In my opinion you do not need to store a pointer to function<void()>, you can simply store function as value (that is have a vector<function<void()>>).

Prior to C++11 and lambdas, to achieve the same effect you would have used a (boost) function also but you would would have used boost::bind with with the address of the fooFn and the first parameter bound to a pointer (or reference) to the Foo object instance.

This would have created an instance of the function that holds all of the information needed to call the fooFn method on the given object. You could then store the instance in a vector to call it at a later time (and had the same problem of making sure no boost::function bound to a destroyed object remains registered)

Edit:

For the sake of completeness, the link to the Boost bind documentation specific for binding members: http://www.boost.org/doc/libs/1_56_0/libs/bind/bind.html#with_member_pointers

What you are doing is actually quite similar only that you are now using a lambda to capture the object pointer and to define the function to be called.

So I see no problem with what you are doing (other then the one I already mentioned).

ds27680
  • 1,993
  • 10
  • 11
  • I do not see the point of the _Prior to C++11_ part of the answer. – nwp Sep 12 '14 at 20:15
  • 1
    The question was tagged as C++11 but in my opinion the only C++11 specifics here are that std::function is now part of STL and the lambda. The problem he has is actually C++11 independent... I also wanted to say that what he is doing is not something that couldn't be done before. And at the same time to say: The old way of doing thigs was fine (no problems). The new way (what you are doing) is actually pretty much the same -> and just fine... And also to describe an alternative way of achieving the same thing. – ds27680 Sep 12 '14 at 20:26
  • Who knows, maybe someone who cannot switch to the latest version of compiler with C++11 support might appreciate this at some point in time... – ds27680 Sep 12 '14 at 20:26
  • The history lesson helps put the rest of the answer in perspective, imo. Thanks! – Conduit Sep 14 '14 at 16:15
-1

You could use an adapter class. This might be overkill for what you're doing, but it may work.

The benefits of doing it this way are:

  1. You don't have to change the original classes. Creating void Foo::registerFnPointer() is ugly.

  2. You don't have to use your static std::vector.

  3. You don't have to deal with function pointers.

So let's say you have two different classes like this:

struct Foo
{
    void fooFn () { 
        std::cout << "Foo::fooFn ()" "\n" ; 
    }
};

struct Bar
{
    void barFn () {
        std::cout << "Bar::barFn ()" "\n" ; 
    }
};

The goal is to put them into a container and call their respective *Fn () member-functions.

An adapter would look something like this:

struct Adapter_Base
{
    virtual ~Adapter_Base () {} ;

    virtual void adapterFn () = 0 ;
};

template <typename T>
struct Adapter : Adapter_Base
{
    T tVal ;

    Adapter (const T &tVal) : tVal (tVal) {}
    void adapterFn () ;
};

template <>
void Adapter <Foo>::adapterFn ()
{
    tVal.fooFn () ;
}

template <>
void Adapter <Bar>::adapterFn ()
{
    tVal.barFn () ;
}

And you could use it like this:

int main ()
{
    std::vector <std::unique_ptr <Adapter_Base> > v1 ;

    std::unique_ptr <Adapter_Base> u1 (new Adapter <Foo> (Foo ())) ;
    std::unique_ptr <Adapter_Base> u2 (new Adapter <Bar> (Bar ())) ;

    v1.push_back (std::move (u1)) ;
    v1.push_back (std::move (u2)) ;

    for (auto &adapter : v1) {
        adapter->adapterFn () ;
    }

    return 0 ;
}
jliv902
  • 1,648
  • 1
  • 12
  • 21
  • What he is currently doing is ok. The std::function is actually doing what your adapter does (It holds on to the instance and function to be called). With considerably less code. He is actually asking if what he does is ok an if there is a better way to acomplish the same. I would not call your proposal a better solution. – ds27680 Sep 12 '14 at 20:11
  • @ds27680 In OP's post, he is adding a new member function into every class. Doing it this way, you do not have to change the original classes. You also do not have to deal with using a static vector or deal with function pointers. – jliv902 Sep 12 '14 at 20:17
  • But the new member function "problem" is not solved by the adapters. You simply moved the code that he wrote in registerFnPointer to main. That he chose to do the registration in a new method of the class (even if debatable) has little to do with his question. He could have written the same (where by the same I mean equivalent) code you wrote in main. Same goes for solving the static vector problem. You moved the vector to main... – ds27680 Sep 12 '14 at 20:35
  • 1
    This is just a terrible re-implementation of std::function. – Puppy Sep 12 '14 at 20:57