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

Subject: Re: [boost] [synapse] Synapse library review starts today December 2
From: Emil Dotchevski (emildotchevski_at_[hidden])
Date: 2016-12-11 19:02:01

On Sun, Dec 11, 2016 at 1:26 PM, Barrett Adair <barrettellisadair_at_[hidden]>
wrote:

> On Fri, Dec 2, 2016 at 8:19 AM, Edward Diener <eldiener_at_[hidden]>
> wrote:
> >      - What is your evaluation of the design?
>
> I don't like that the invocation mechanism is single-threaded by default.
> I think the thread_local approach is interesting, but it sounds ripe for
> confusion in application code.
>

It's not single-threaded by default, I've spent a lot of effort to make
Synapse work in multi-thread environment without special initialization.
For example, libraries can emit signals without caring whether they're
linked into a single- or multi-threaded program (also emitting signals
itself does not require linking of Synapse itself).

The design choice to queue emitted signals asynchronously to be polled
synchronously by other threads (see
http://zajo.github.io/boost-synapse/create_thread_local_queue.html) is
motivated by experience using other signal libraries. It is also very
efficient, as emit<S> is queued only in threads that currently connect
specifically the signal S.

> >      - What is your evaluation of the implementation?
>
> This code style is quite foreign to me, but it seems consistent.
>
> The mix of Boost and std libraries seems arbitrary (e.g. boost::shared_ptr
> and <thread>).
>

<thread> is an implementation detail, not part of the public interface,
subject to change without notice, etc.

boost::shared_ptr is used in the public interface because Synapse is
formatted for Boost review.

> The Boost.Function dependency appears redundant. The callback is
> effectively being erased twice. std::invoke or equivalent could serve as a
> replacement.
>

It may be possible to remove function as a dependency, thanks for the note.

> I don't much like signal_traits.hpp and the limitations it imposes.
> Arity is capped at 4 parameters, and the signal type must be passed
> explicitly. The latter is surely good for compile times, but I think it
> would
> be nice to have a deduced signal type version that could still support
> stateful lambdas and member functions.
>

Arity should be extended for sure, but the reason for signal_traits is to
help users write generic code that works with signals.

> This line was disappointing:
>
> "requires compiler support for the following C++11 features"
>
> I have seen this in existing Boost library documentation, so there is
> precedent, but I do not like it at all. I much prefer to see a range of
> tested compiler versions, even if it's only a couple. Continuous
> integration
> services like Travis and Appveyor are free, and very useful toward
> this end.
>

As I mentioned elsewhere, in my reading of the documentation
boost::thread_specific_ptr is not thread-safe on platforms without built-in
support for concurrent initialization of local static objects. This is the
reason why Synapse does not use boost::thread_specific_ptr.

Otherwise I do agree that it would be better to support older compilers. If
anyone can recommend a suitable alternative to built-in thread_local and
concurrent initialization of static objects, I'm all ears.

> P.S. For whatever it's worth, I did fork the Synapse code and implement
> tentative variadic support in connect.hpp and emit.hpp (though I did not
> get around to translate.hpp).
>
> https://github.com/zajo/boost-synapse/compare/master...badair:master

It seems you also removed the dependency on function. Seems good.

> It appears to work as-is with the logger example, but similar work would
> need to be done in translate.hpp to make it fully variadic. I used
> my proposed CallableTraits library with the intention of implementing
> deduced signal types, but I ran out of willpower. Please forgive the
> aggressive formatting.
>

Formatting-shformatting. :)

Thanks,
Emil