Re: [boost] [synapse] Synapse library review starts today December 2
On Fri, Dec 2, 2016 at 8:19 AM, Edward Diener <eldiener@tropicsoft.com> 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.
On Sun, Dec 11, 2016 at 1:26 PM, Barrett Adair <barrettellisadair@gmail.com> wrote:
On Fri, Dec 2, 2016 at 8:19 AM, Edward Diener <eldiener@tropicsoft.com> 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
participants (2)
-
Barrett Adair -
Emil Dotchevski