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]>
> On Fri, Dec 2, 2016 at 8:19 AM, Edward Diener <eldiener_at_[hidden]>
> > - 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
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
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
> 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
> 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).
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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk