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


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk