On Sat, Dec 3, 2016 at 7:06 AM, Klemens Morgenstern < klemens.morgenstern@gmx.net> 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.... http://zajo.github.io/boost-synapse/adding_custom_signals_to_Qt_objects_with... http://zajo.github.io/boost-synapse/using_meta_signals_to_connect_to_a_C-sty...
- 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