Boost logo

Boost :

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

Boost list run by bdawes at, gregod at, cpdaniel at, john at