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

Subject: Re: [boost] Synapse library review starts today December 2
From: Emil Dotchevski (emildotchevski_at_[hidden])
Date: 2016-12-04 05:44:35

On Sat, Dec 3, 2016 at 7:06 AM, Klemens Morgenstern <
klemens.morgenstern_at_[hidden]> wrote:

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

In synapse/dep/smart_ptr.hpp you can change which shared_ptr/weak_ptr you
want to use, though this should probably be controlled by a configuration
macro. However, if this is a Boost library, it should use boost::shared_ptr.

The dependence on C++11 is only for thread_local and for correct concurrent
initialization of static objects. The library is still very useful without
these, it's just that the (optional) interthread communication support
won't work.

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

Using shared_ptr to hold on to connections is a design choice.

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

See http://zajo.github.io/boost-synapse/questions_and_answers.html.

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

That's what the null_deleter in shared_ptr is for.

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

Do you mean provide thread affinity for the connection objects? That can be
done.

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

Do you mean that you want threads other than the connecting thread to be
able to poll for queued signals? Why not just connect from these other
threads?

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

connect<>() does deduce the type of the emitter, in order for
connection::emitter<>() to be type-safe (see "Emitter type safety" in
http://zajo.github.io/boost-synapse/Tutorial.html). However, the emit<>()
machinery has no use for the type of the emitter, meaning, there is nothing
to be made type-safe by deducing it.

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

Obviously my company name would not appear in the documentation if this
becomes a Boost library. I don't think that using quickbook is a Boost
requirement, but perhaps I'm wrong.

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

Synapse does not claim to be a "better" Signals2; its usefulness is not as
an alternative to Signals2 but as a solution to problems Signals2 can not
solve. Here are three examples of this, though I have many more:

http://zajo.github.io/boost-synapse/handling_events_from_an_OS_message_pump.html
http://zajo.github.io/boost-synapse/adding_custom_signals_to_Qt_objects_without_MOCing.html
http://zajo.github.io/boost-synapse/using_meta_signals_to_connect_to_a_C-style_callback_API.html

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

Thank you!

Emil