On Sun, Dec 4, 2016 at 4:28 AM, Klemens Morgenstern < klemens.morgenstern@gmx.net> wrote:
Am 04.12.2016 um 11:44 schrieb Emil Dotchevski:
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.
Not as I understand this sentence: "It also means using the C++ Standard Library where applicable." http://www.boost.org/development/requirements.html
If the rules say that Boost libraries shouldn't use boost::shared_ptr but std::shared_ptr if available, I'll comply (though I can't imagine why this would be a good idea.) Well don't take my word for it, it's just how I understand it. Boost
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
I considered using boost::thread_specific_ptr but to my understanding its initialization must be complete before it is used by different threads. This makes it impossible to use in Synapse because it uses thread local storage per type inside function templates, which would require C++11 concurrent initialization of static objects (please do correct me if I'm wrong). So, I opted for a cleaner implementation using C++11 only for these two features (thread_local and concurrent initialization of static objects). Ok, that might be the case, I don#t know. I was just thinking in terms of choosing one standard for everything.
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.
Strictly speaking, using shared_ptr does use move since the shared_ptr itself is moveable. :)
I guess we'll agree to disagree, in my opinion refcounting is more appropriate for connections compared to move semantics. Well that's more of a philsophical question, but here's why I wouldn't
Am 04.12.2016 um 23:20 schrieb Emil Dotchevski: libraries are intended to give you functionality the standard doesn't and not to be an alternative STL. think so: a connection is attached to two objects and thus might be shared between those two but not any more. Thus there's no real need to refcount it, because it won't be shared with anything else.
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.
What I mean is that I can be persuaded that the benefits of this feature in signals2 somewhat outweight the added complexity, but in Synapse the return type is used to tell apart different signals with otherwise the same signature, which is quite practical in my experience. Perhaps I'm missing some important use cases but I've never missed that particular feature.
I think if you argue that better, no one would complain. E.g. saying it's more of an event-approach, it reduces complexity and (maybe) increases performance.
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?
Is there something like weak_ptr that works with unique_ptr? The point of passing weak_ptr to connect is that emit won't call the connected function after the emitter (or the target) have expired, even if the connection object is still afloat. When it is impossible to use weak_ptr for this, then the only option to stay safe is to control the lifetime of the connection object (and pass a raw pointer to connect<>) .
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_co nnect_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
boost::signals2::connection connect(Func f) {return impl<Signature>().connect(f);} template
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?
I suppose if it is elaborate enough you'd rediscover Synapse, but with a different low level machinery, though I suspect that both libraries contain incompatible intricacies which make sense in the different approaches they take, since non-intrusiveness (in the way Synapse is non-intrusive) was not a design goal of Signals2. That may be true; there are lots of problems, which look somewhat easy from the outside but if you try to actually implement a solution you discover a horrible mess of details. I'm just wondering that, and I
There is not. You could use a custom deleter for that though. think you're in a situation where it would be very beneficial to explain this more, since boost.signals2 exists. Btw. since I was the first one to write a review and I voted no, please let me clarify: I like the idea of the library and don't think something like that should never be in boost. I do think it can be useful if some of the weak spots can improved etc. So it's more of a not yet, than a never vote on my part.