Will you review my addition to boost::signals?

Started by
2 comments, last by MaulingMonkey 15 years, 4 months ago
I think I still have a thread floating around here where I asked about some boost::signals stuff. Thanks to those who responded, it helped me a lot. Anyway, I've been playing around with it for the past 2 days and decided I wanted to make it easy to connect member functions of objects to signals. Below is the code I've come up with. Note that I'll definitely add support for signals with 0-9 parameters, I just wanted to do one with 3 parameters first so I could plan out how I'm going to implement things. I'd greatly appreciate it if you could take a moment to look over my code and let me know if there are things I'm doing that I shouldn't be, if there are things I'm not doing that I should be, or if I can improve it in any way. Thanks! Signal.hpp (I still need to implement The Rule of 3 and such, but I just want the class's core reviewed)
#ifndef SIGNAL_HPP
#define SIGNAL_HPP 1

#include <boost/signal.hpp>
#include <boost/bind.hpp>

template <typename Function> struct Helper;

template <typename R, typename T1, typename T2>
struct Helper<R (T1, T2)>
{
    static const unsigned int arity = 2;
    typedef R result_type;
    typedef T1 arg1_type;
    typedef T2 arg2_type;
};

template <typename Signature>
class Signal
{
    private:
        boost::signal<Signature> internalSignal;

    public:
        typedef typename Helper<Signature>::result_type result_type;
        typedef typename Helper<Signature>::arg1_type arg1_type;
        typedef typename Helper<Signature>::arg2_type arg2_type;

        // for global or static member functions
        void connect(Signature* function)
        {
            internalSignal.connect(function);
        }
        
        // for non-static member functions
        template <typename C>
        void connect(result_type (C::*member)(arg1_type, arg2_type), C* object)
        {
            internalSignal.connect(boost::bind(member, object, _1, _2));
        }
        
        result_type operator() (arg1_type t1, arg2_type t2)
        {
            return internalSignal(t1, t2);
        }
};

#endif


Trackable.hpp (I may just move this into Signal.hpp since it's only 2 lines)
#ifndef TRACKABLE_HPP
#define TRACKABLE_HPP 1

#include <boost/signals/trackable.hpp>

typedef boost::signals::trackable Trackable;

#endif


main.cpp (I made this just for testing reasons)
#include <iostream>

#include "Signal.hpp"
#include "Trackable.hpp"

struct MyEmitter
{
    Signal<void (int, bool)> emit;
};

struct MyStruct : Trackable
{
    void print(int i, bool b)
    {
        std::cout << "Member " << i << ' ' << b << std::endl;
    }
};

void global(int i, bool b)
{
    std::cout << "Global " << i << ' ' << b << std::endl;
}

int main()
{
    MyEmitter emitter;
    
    emitter.emit.connect(&global);
    
    emitter.emit(1, false);
    
    {
        MyStruct obj;
        emitter.emit.connect(&MyStruct::print, &obj);
	    
        emitter.emit(2, true);
    }
    
    emitter.emit(3, false);

    system("pause");
}


[Edited by - MikeTacular on November 30, 2008 11:34:55 AM]
[size=2][ I was ninja'd 71 times before I stopped counting a long time ago ] [ f.k.a. MikeTacular ] [ My Blog ] [ SWFer: Gaplessly looped MP3s in your Flash games ]
Advertisement
This is already handled pretty easily with boost::bind.
Quote:Original post by mikeman
This is already handled pretty easily with boost::bind.

He's already using boost::bind, the point of his additions is to make the syntax a little less painful.
NextWar: The Quest for Earth available now for Windows Phone 7.
The reason boost::signals doesn't already do this is already is documented here -- those figures only include trying to autohandle const and non-const references, trying to automate when placeholders should or should not be introduced in a generic way would be even worse, unless you special case it as an all-or-nothing type of thing (in which case you're still looking at 20 overloads).

You're also missing a version of connect for functors -- meaning you can't manually use boost::bind with your class, currently. boost::signal<>::connect also returns a boost::signals::connection, which can be useful. You don't have scoped_connect()ions, [scoped_]disconnect()ions, etc handled... easily another 60 overloads, even for your all-or-nothing approach... for a total of 80 already.

Now, the additional reasons that I haven't done something like this myself:

1) This is a lot of work when the improvements only touch one library (which is how it will have to be to eliminate the manual call to bind). I spend a lot more time using boost::asio, for example -- this wouldn't help me at all there, but will still increase my compile times.

2) In the end, you're not really eliminating all that much. Your final construction will probably be more code than you ever end up writing as bind statements. You're really not eliminating much after all: bind(,_1,_2) <--- the sum of all eliminate-able text right there, with your current code, which is literally less than 1/100th of the code in your current Signal.hpp alone, even with all the stuff I've pointed out is missing.

3) In the end, you're actually writing a lot more. boost::bind is battle tested, is already debugged, and works. Your Signal class isn't -- nor would mine be. It's also another thing that would need to be maintained and understood.

I'm not saying don't do it -- you may have good reasons for doing it -- but just to list some things to keep in mind if you find yourself tempted to let runaway feature-itus take over.

This topic is closed to new replies.

Advertisement