Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lambda support for signalling #132

Closed
huwpascoe opened this issue Aug 27, 2018 · 10 comments
Closed

lambda support for signalling #132

huwpascoe opened this issue Aug 27, 2018 · 10 comments
Assignees
Labels
enhancement accepted requests, sooner or later I'll do it invalid not a bug/not an issue/not whatever you want

Comments

@huwpascoe
Copy link

This syntax especially

struct S {
    int capture;
    void bar(int, char) { /* ... */ }
};
S instance{ x };
signal.sink().connect<S, &S::bar>(&instance);

Would be very useful to support lambda syntax (c++ converts to the very same pattern internally, I think)

signal.sink().connect([x](int, char) { /* ... */ });
@indianakernick
Copy link
Contributor

indianakernick commented Aug 27, 2018

The signal stuff is made to be fast. It’s designed to avoid allocations. If you want a lambda with state, you have to store the state somewhere. std::function will allocate space for that state but entt::Delegate will not. Supporting this feature would introduce allocations everywhere.

If your lambda is state-less then it can be cast to a function pointer. Unary plus operator can do that.

signal.sink().connect<+[] (int,char) {
  /* ... */
}>();

I’m not sure how you would go about disconnecting that though.


Although, we could make the user in charge of the state. Maybe if the user passes in a pointer to an object, we could grab a pointer to operator() automatically.

S instance{x};
signal.sink().connect<S>(&instance);

All we can really do is make the workaround in the original post a little cleaner.

@skypjack
Copy link
Owner

skypjack commented Aug 27, 2018

What @Kerndog73 said is more or less what I'd have said, but for the fact that lambdas cannot be used as template parameters in any case as far as I remember.

The fact is that SigH is all about template and it isn't in charge of the lifetime of the listeners. Supporting lambdas the way you're proposing requires a completely different design and some allocations around, more or less what Emitter offers. On the other side, SigH allocates only to store erased wrappers around a couple of pointers and the idea is to introduce custom allocators in future so as to reduce the performance hit due to allocations and have a finer control on wrappers.

If you want to create a signal class that accepts lambdas, probably SigH isn't what you're looking for, that's all. The idea behind this class is all the ways different from that.

@indianakernick
Copy link
Contributor

indianakernick commented Aug 27, 2018

Oh, of course lambdas can’t be used as template parameters! Every lambda expression has a different type. It just wouldn’t make sense. Silly me! I should have compiled that.

@skypjack skypjack self-assigned this Aug 27, 2018
@skypjack skypjack added invalid not a bug/not an issue/not whatever you want question open question enhancement accepted requests, sooner or later I'll do it and removed question open question labels Aug 27, 2018
@skypjack
Copy link
Owner

skypjack commented Aug 27, 2018

I'm closing the issue because it's invalid, something we cannot implement within SigH, I'm sorry.
Feel free to continue the discussion here if you have any proposal or suggestion. Thank you.

@huwpascoe
Copy link
Author

Don't worry it's not vital, wondered if it was overlooked. 👍 @Kerndog73 for the +[] operator

@Snektron
Copy link

Snektron commented Jul 8, 2019

I understand signals aren't in charge of the lifetime of the listener, however i don't think writing

bool thing_happened = false;
auto listener = [&thing_happened]{
  thing_happened = true;
};
sink.connect<&decltype(listener)::operator()>(&listener);

is quite nice. A possible alternative would be to add an overload of connect that automatically deduces operator() of its parameter:

template <typename Listener>
connection connect(Listener* listener) {
  connect<&Listener::operator()>(listener);
}

This would allow rewriting the first snippet as

bool thing_happened = false;
auto listener = [&thing_happened]{
  thing_happened = true;
};
sink.connect(&listener);

which seems a lot nicer in my opinion.

@indianakernick
Copy link
Contributor

@Snektron I was thinking about this just the other day!

@skypjack
Copy link
Owner

skypjack commented Jul 8, 2019

@Snektron @Kerndog73 a lambda created on the stack and attached this way would result in a dangling pointer and therefore a crash.

@Snektron
Copy link

Snektron commented Jul 8, 2019

Wouldn't the same thing happen with a listener created on the stack?

@skypjack
Copy link
Owner

skypjack commented Jul 8, 2019

@Snektron indeed, of course. @Kerndog73 correctly commented the decisions behind sigh a while ago:

The signal stuff is made to be fast. It’s designed to avoid allocations. If you want a lambda with state, you have to store the state somewhere. std::function will allocate space for that state but entt::Delegate will not. Supporting this feature would introduce allocations everywhere.

The fact is that the delegate class (and therefore sigh, that you can see as a multi-delegate class in a sense), doesn't allocate nor it has space to store aside lambdas or instances of any type. It gets the job done with just a couple of pointers and that's all. On the other side, fully supporting lambdas would require to design something similar to an std::function that does allocate instead in some cases and for obvious reasons. However, if what I need is an std::function, I can use it directly, it doesn't make much sense to design a drop-in replacement with no benefits at all and with the exact same API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement accepted requests, sooner or later I'll do it invalid not a bug/not an issue/not whatever you want
Projects
None yet
Development

No branches or pull requests

4 participants