not able to connect to lambda with non empty capture
Closed this issue · 3 comments
Looks like it is not possible to connect a signal to a lambda with a non empty capture:
int main()
{
vdk::signal<void(int)> sig;
sig.connect([=](int arg) { (void)arg; });
return 1;
}
error: no matching member function for call to 'connect'
sig.connect([=](int arg)
~~~~^~~~~~~
I guess this is the same problem with trying to connect to a std::function by value:
std::function<void(int)> f = [](int arg)
{
(void)arg;
};
sig.connect(f); // passing f by value does not work (nor does std::move(f))
In those cases, I guess the signal needs to take a copy of the lambda/function because it has some data in it.
So to be a bit more precise, you can do:
auto lambda = [=](){};
sig.connect(&lambda);
but not:
sig.connect([=](){}); // but sig.connect([](){}) works (no capture)
I think that would be nice to have a version of connect that takes ownership of the passed functor.
Hi! Thanks for your feedback and your contribution.
I have been planning a major rewrite of the library for very long time but I couldn’t really find enough time to think through it carefully. Basically, I want to rewrite it from scratch, improving some of its aspects and design decisions based on real life usage statistics and suggestions from other users. I have used it extensively in several projects and I haven’t noticed any issues regarding the correctness of the code itself. However, based on long experience I’ve concluded:
• Self-tracking is really a bad idea. In any non-trivial project, design that requires self-tracking complicates code and architecture A LOT. So, I wouldn't recommend using it even though it works correctly as far I observed.
• Creating two separate sub-versions of the library for single-threaded and multi-threaded use was a mistake. First, playing with macros this way often leads to violating one definition rule. This can happen if you specify library mode per-#include, not for the entire project. I've also tried using just two separate classes with different threading policy. However, in any non-trivial project it is hard to track all those interconnections between signals and slots of differently “threaded” types, so you end up connecting multi-threaded and single-threaded entities. In fact, this is why initially I decided to use macro to switch between library modes, instead of just creating two separate signal classes. That way I hoped that the library mode would be set for the entire project, avoiding the mix of differently “threaded” classes. However, I guess I should have created only the multi-threaded version. I think some similar reasons stand behind design of std::shared_ptr that doesn't have a version with non-thread-safe reference counter.
Regarding unit tests… Initially, I uploaded the library only because some people I knew asked me to upload it and they asked me to write detailed documentation about its internal workings and design rational. They never asked me to write unit tests, so I didn't. However, you are right, I should have written them. So, in the new rewrite I will definitely write unit tests. Though, properly testing multi-threaded code is very tricky.
Also, based on my experience of using the library, I've come to the conclusion that it should be thread-aware, meaning that it should support cross-thread slot invocations. However, I never figured out how to make it non-intrusive, so that it would not dictate a user what kind of event loop to use and what kind of thread implementation to use. Moreover, proper cross-thread calls would require that some event queue is associated with each thread and therefore with each object that lives in that thread. And any given object must have exactly one such event queue associated with it. To ensure that, it looks like we have no choice but deriving each object that wishes to receive cross-thread calls from some base class. That class would force us to have exactly one event queue associated with each object and it would handle all underlying mechanisms. Moreover, it seems that Qt’s detection of cross-thread calls is a good idea. This way, signal emission can detect what target thread the object belongs to, and marshal arguments if needed. And again, to ensure exactly one thread affinity for the object and to be able to detect that thread affinity, it seems that we are forced to include in the design a base class for all receivers. After that, it seems that using shared_ptr for auto disconnection is not that necessary, since that base class would have to track all connections anyway.
Regarding lambdas and functors. The initial design idea was that you can disconnect any slot exactly the same way you connected it. So, you don’t have to store some “cookie” object that represents a connection somewhere (like in boost::signals2), just to be able to disconnect it later. However, it is not possible to do with lambdas, because lambdas are unique and cannot be compared by definition. Also, by far not all functors provide comparison operators either. So, if I want to disconnect a slot later I should keep that connection object around, probably many of them. Many means a collection, like vector, and from this point it starts look ridiculously. The signal itself is basically a collection of connections, and yet we have another collection to keep track of the first collection. This is why I decided not to take ownership of lambdas and use them only when it is feasible to do by their address, which is by the way, can be compared. However, it seems that a lot of people are missing the feature of connecting lambdas with taking ownership, so I will probably add that in the new rewrite.
Also, taking ownership of lambdas has another implication. Since lambdas with captured variables can be of arbitrary sizes, underlying memory allocator cannot be used anymore. This is so, because it assumes that all internal connection objects are of the same size, and lambdas would break that assumption. Creating allocator per signal would not be feasible anymore, because each signal usually has not that many connections to be able to compact them based on their sizes (by similar sizes). So, it implies that the library would require some global allocator used by all signals, that could compact connection objects of similar sizes together. In turn, this implies that there won’t be any constructor for signal class that takes a preferable number of connected slots to allocate memory in advance.
And the last thing you didn’t asked about, but just in case. A lot of people asked me to make signals movable. However, anything that can be accessed from multiple threads and is movable in the same time – is extremely dangerous. So, I decided to avoid it.
I will use your unit-tests in the new version, since the interface of the signal class itself will not be changed that much. However, there is no point in merging it here, since it all will be rewritten anyway. In the same time, if you have any thoughts about any of this, or maybe some suggestions based on your use case, or any previous usage of some version of signals libraries, I would very much appreciate if you shared your thoughts with me. Thanks again.
Not too sure about implementation details, but here are some comments regarding my past and current experience using signals/slots:
- I wrote Qt application for 10 years, and their system work great. Auto-disconnect and thread safety was a must have for any decent size application.
- I am now targetting more embedded stuff (on top of FreeRTOS) and the application is mostly single threaded, hence my interest in VDK signal lite. I mean, the part that is going to need sig/slot is single threaded. I'd use sig/slot mostly as a way to connect several callbacks to an "event".
- I can probably live without auto-disconnect. Ideally I would use it, but I am not sure how it fits in the current architecture (e.g. we don't have shared_ptr anyware right now, we use unique_ptr everywhere).
- Being able to connect any lambda/std::function... is a must have. 99% of the time we have non empty capture in the lambda, so the signal needs to take ownership.
- Binary size (and memory use) is really important. More so than calling performance because we use sig/slot for more high level things. If super performant code is needed, usually we only need one callback and thus can just roll our own system (e.g. taking the callable as a templated parameter or something).