Re: [boost] Synapse library review starts today December 2

Subject: Re: [boost] Synapse library review starts today December 2
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-12-03 10:06:46

> For your review you may wish to consider the following questions:
>
>      - What is your evaluation of the design?
The way I see it, it tries to solve a specifiy style problem for signals 
in a very simple and straight-forward way.

But: if it uses C++11 (like with type_traits) why not go all the way and 
std::shared_ptr instead of boost::shared_ptr? Why are there no 
move-operations for example for connections. It doesn't make sense to 
put this into a shared_ptr, when you can make it movable, while 
disabling copies.

Also: why can't return values be combined as in boost.signals2? That 
should at least be an optional feature.

Now the slot-lifetime is determined by weak_ptr, if I understand 
correctly. I don't think that's generic enough at all, there should be 
more solutions. One would be, that target can inhert some sort of 
slot-type, which will handle that. That way I could use the signals on 
elements on the stack, and also do this unintrusively:

struct signalable_vector : std::vector<int>, slot_type {};

Some solution for unique_ptr is needed. A solution would be to implement 
a synapse::unique_slot_ptr which implements that.

So, I think to use weak_ptr isn't bad, but having it as the only 
possibility isn't sufficient.

If I understand it correctly, interthread signaling will be used if emit 
is called on a different thread than connect was? I think that's 
probably an alright default, but bad design if it's always the case. 
Either provide a thread-affinity for each object, which can be changed, 
or add a dispatcher to make it explicit.

I would personally prefer a dispatcher, which would not only allow to 
dispatch it to a std::thread through thread_local_queue but also to 
something like a boost::asio::io_service. The io_service can actually be 
executed from several threads, so you don't know which one will actually 
poll it.

Something like that (based on the example from "Connecting Signals").

my_button b;
std::thread thr(...); //this thread will hold the executor
auto c = synapse::connect<button_clicked>(&b,
   synapse::dispatch(thr, boost::bind(&target::do_something,t.get()),t));

and with an io-service:

boost::asio::io_service ios;
auto c2 = synapse::connect<button_clicked>(&b,
   synapse::dispatch(ios, boost::bind(&target::do_something,t.get()),t));

>      - What is your evaluation of the implementation?
The code style (intendation) is very weird.

emit is declared with 0-4 parameters, which should be a variadic 
template functions.
Also, why is the first element void const * and not a template? This 
type must be checked at compile-time, if I want void* I can use moc. 
That is not acceptable for any C++ library; you can cast it to void* in 
the function, but you must not have a void* as part of the public interface.
Same goes for translate.

>      - What is your evaluation of the documentation?
No problems there, made sense. Not sure why it's not boostbook/quickbook 
though. Having your companies name in there feels a bit misplaced.
>      - What is your evaluation of the potential usefulness of the
>        library?
I don't think it's that useful, but I guess I would still use 
boost.signals2 and boost::asio::io_service for interthread signaling. 
I'm not convinced this should be a distinct library, and not a feature 
of boost.signals2.
>      - Did you try to use the library?  With what compiler?  Did you
>        have any problems?
MinGW 6, the build rules (in term of jamfiles) are not yet correct. It 
can be built, but the text cannot be executed in the typical boost.build 
style.
>      - How much effort did you put into your evaluation? A glance? A
>        quick reading? In-depth study?
Tried a simple example and looked through the code. ~3h.
>      - Are you knowledgeable about the problem domain?
Used libsigc++, Qt, boost.signals(2) and had custom solutions.
>
> And finally, every review should attempt to answer this question:
>
>      - Do you think the library should be accepted as a Boost library?

No, it should not. The library has a signal approach that is different 
enough from boost.signal, that I could see it as a part of boost. But is 
has to many big design and implementation flaws as of now. I don't think 
it has to be redesigned from the ground up, but some effort needs to be 
put into redesigning some parts of it.

But really having a void* as part of a public interface would've given 
you a no, even if everything else was perfect with this library.

And I am also not convinced, that this should have it's own 
implementation, instead of being built with boost.signals2 as backend.