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

Subject: Re: [boost] [synapse] Synapse library review starts today December 2
From: Barrett Adair (barrettellisadair_at_[hidden])
Date: 2016-12-11 16:26:56

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 know anything about Signals2, but I find the "non-intrusive"
argument to be compelling.

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.

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

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

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.

I browsed the test cases, and they seem rather in-depth. I think some
comments in this code are warranted.

>      - What is your evaluation of the documentation?

The tutorial is very well done. Overall, the documentation is very solid.
I enjoyed reading it.

Quickbooks and syntax highlighting would be nice.

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.

>      - What is your evaluation of the potential usefulness of the
>        library?

It appears that it could be useful to Qt users. I personally would not
find this useful.

>      - Did you try to use the library?  With what compiler?  Did you
>        have any problems?

I ran the logger example with GCC 5.3 on Ubuntu 16.04. No problems.

>      - How much effort did you put into your evaluation? A glance? A
>        quick reading? In-depth study?

4 hours combined of reading the documentation and playing around with
the implementation code.

>      - Are you knowledgeable about the problem domain?

My experience with C++ signal libraries is limited. I am comfortable
using the Intel TBB flow graph library, which has some degree of overlap.
I have a bit of experience with event-driven programming in other
languages. I am knowledgeable about C++ callable types and compiler
support for their variations.

>      - Do you think the library should be accepted as a Boost library?

This is my first formal review participation, I don't have a library in Boost,
I'm not familiar with Signals2, and I do not have a use case for Synapse, so
I'm not going to answer this question. Please consider this message as a
"technical comment" instead.

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