-
Notifications
You must be signed in to change notification settings - Fork 29
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
RFC: Less complex, footgun free API #104
Comments
I think you meant to write "stack". :)
Sounds good but to keep things simpler and limiting public API, I would suggest keeping everything Stack* internal (doc hidden) except for the macro as that should be the only way to do stack-based listeners. |
Sounds good. (as usual before finally merging this after implementation, we should of course test it in various downstream cases to make sure nothing breaks weirdly (e.g. interactions of lifetimes, etc.)) |
Minimal amount of changes to make EventListener a heap-allocated type again. The existence of the EventListener implies that it is already listening; accordingly the new() and listen() methods on EventListener have been removed. cc #104 Signed-off-by: John Nunley <dev@notgull.net>
cc smol-rs/event-listener#104 Signed-off-by: John Nunley <dev@notgull.net>
Minimal amount of changes to make EventListener a heap-allocated type again. The existence of the EventListener implies that it is already listening; accordingly the new() and listen() methods on EventListener have been removed. cc #104 Signed-off-by: John Nunley <dev@notgull.net>
Minimal amount of changes to make EventListener a heap-allocated type again. The existence of the EventListener implies that it is already listening; accordingly the new() and listen() methods on EventListener have been removed. cc #104 Signed-off-by: John Nunley <dev@notgull.net>
Minimal amount of changes to make EventListener a heap-allocated type again. The existence of the EventListener implies that it is already listening; accordingly the new() and listen() methods on EventListener have been removed. cc #104 Signed-off-by: John Nunley <dev@notgull.net>
Minimal amount of changes to make EventListener a heap-allocated type again. The existence of the EventListener implies that it is already listening; accordingly the new() and listen() methods on EventListener have been removed. cc #104 Signed-off-by: John Nunley <dev@notgull.net>
cc smol-rs/event-listener#104 Signed-off-by: John Nunley <dev@notgull.net>
Minimal amount of changes to make EventListener a heap-allocated type again. The existence of the EventListener implies that it is already listening; accordingly the new() and listen() methods on EventListener have been removed. cc #104 Signed-off-by: John Nunley <dev@notgull.net>
Minimal amount of changes to make EventListener a heap-allocated type again. The existence of the EventListener implies that it is already listening; accordingly the new() and listen() methods on EventListener have been removed. cc #104 Signed-off-by: John Nunley <dev@notgull.net>
Continued from the discussion in #100
The APIs of v3.x and v4.x increase complexity at the cost of adding more footguns. Originally I'd wanted to keep the API small and simple, but this appears to have introduced a handful of different ways to mess up (e.g. panicking when creating a
new
EventListener
withoutlisten
ing on it). In addition, v4.0 (#94) introduced a not-insignificant amount of overhead over the v3.x API. Therefore, as much as it pains me to have three breaking changes in this crate in such a short span of time, I think we need a better API.Note: I am bad at naming things. None of these names are final.
Reference-Level Explanation
The idea I proposed in #100: is as follows: add an API based around a
Listener
trait that would look like this:This
Listener
trait can do anything that the currentEventListener
type can do. It can beawait
ed or it can be used to block the current thread. Alternatively, it can be discarded.To emulate the previous
2.x
API, we would have anEventListener
type. This is a heap-allocated event listener that can be moved freely.It would be used like this:
However, this is inefficient, as it preforms a heap allocation every time
listen()
is called. We also provide an API that allows one to use the stack instead of the heap, at a slightly higher complexity cost. To start, you create aStackSlot
, which contains all of the state of theEventListener
but stored on the stack. After being pinned, it can be transformed into aStackListener
.In addition, a macro will be provided that creates a
StackSlot
from anEvent
and automatically pins it to the heap.The example from above can be modified to look like this:
@smol-rs/admins Thoughts on this?
The text was updated successfully, but these errors were encountered: