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

Subject: Re: [boost] Synapse library review starts today December 2
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-12-04 07:28:55

Am 04.12.2016 um 11:44 schrieb Emil Dotchevski:
> On Sat, Dec 3, 2016 at 7:06 AM, Klemens Morgenstern <
> klemens.morgenstern_at_[hidden]> 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.
Not as I understand this sentence:  "It also means using the C++ 
Standard Library where applicable."
http://www.boost.org/development/requirements.html

> 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.
If you have the library for the most part in C++98 and some extra 
features in C++11 that does make sense. But I think if you want to do 
that, go for one standard, and thus either use everything from C++11 or 
http://www.boost.org/doc/libs/1_62_0/doc/html/thread/thread_local_storage.html

>
>> 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.
I've gathered that much, I'm just questioning it ;). It makes sense if 
you are pur C++98, but not if you have C++11. And I do think this is a 
bad choice for C++11.
>> 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.
I know, but that doesn't answer the question, really. Because the 
reasoning can also be applied to boost.signals2 also, because signals in 
general don't care how many functions are connected.
>
>> 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.
How does that help with unique_ptr?
>
>
>> 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.
Yes, that is what I meant. Alright, seems like I missed that.
>
>
>> 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?
Well because I might have 3 thread polling work from the io_service - 
how would I know on which one the signal ends up?
>
>> 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.
Well it should, so there's real type-safety, as discussed in the other 
E-Mails.
>
>>       - 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.
No it isn't and that's not a big issue, but that just caught my eye.
>>       - 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.html
> http://zajo.github.io/boost-synapse/adding_custom_signals_to_Qt_objects_without_MOCing.html
> http://zajo.github.io/boost-synapse/using_meta_signals_to_connect_to_a_C-style_callback_API.html
>
How about:

template<typename Signature>
inline boost::signals2::signal<Signature> & impl()
{
    boost::signals2::signal<Signature> sig;
    return sig;
}

template<typename Signature, typename Func>
boost::signals2::connection connect(Func f) {return 
impl<Signature>().connect(f);}

template<typename Signature, typename ...Args>
void emit(Args... args) {return impl<Signature>()(args);}

Let's say I build a more elaborate version of this - how does that 
compare to synapse?