-
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
feat: Add tagged notifications #52
Conversation
This will be used to replace the notify_* family of functions.
I missed that this is needed for notification propogation.
Sets up tagged events to use the Notification trait.
3e6efed
to
5a9cfbb
Compare
|
Fixes the following issues: - Failed to build on MSRV due to Unpin bound - Failed to build on no_std, as I didn't import Box - Failed to build on portable-atomic, as Arc didn't implement Unpin
2ade5ae
to
723c328
Compare
I think you've messed up the negation of "sealed" here. Also, if we don't seal it, it might be a good idea to split up the |
This PR seals these two traits to prevent breaking changes from causing semver breaks. It also adds documentation to both traits.
Thanks for the catch! I've decided to seal it anyways, since you can do whatever you want using the combinators. |
src/no_std/node.rs
Outdated
@@ -12,8 +13,10 @@ use alloc::boxed::Box; | |||
use core::num::NonZeroUsize; | |||
use core::ptr; | |||
|
|||
pub(super) type GenericTags<T> = Box<dyn FnMut() -> T + Send + Sync + 'static>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really a good idea to require an allocation for usage? (why not make it generic in such a way that T is a function itself, which would be more general and probably also faster)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it's necessary. This structure is stored directly inside of Queue<T>
, so we can't use the function as a parameter without it propagating all the way to Event
and EventListener
.
In addition, this is on a cold path, so it's unlikely to happen in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm so why can't we parameterize Event
and EventListener
with that anyways and create type aliases for this case? It would blow up the types a bit, but I don't think it would be bad in practical application
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closure stored in these boxes isn't a user provided closure; it's this closure. Until we have TAIT, we can't actually get a type for this closure, therefore we have to box it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh... but why do we have to use such a closure and not package it into a custom struct and idk... have it implement Iterator
or such (or why not just a counter and a generating function?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I've replaced the boxed closure with a Vec
of tags. It should have the same effect.
This should hopefully reduce the amount of allocation on the cold no_std path
The MSRV failure looks spurious to me; running |
I'm not like 100% sure about the design, but it looks ok to me now. |
Supersedes #40 by adding tags to the event. Similarly to #40, this PR:
T
parameter toEvent
andEventListener
that is()
by default.wait()
and friends returnT
orOption<T>
instead of()
andbool
.EventListener
as a future returnsT
instead of()
.Unlike #40, I wanted to use a more sustainable method for notifying the event. Introducing the Notification trait. Its API looks like this:
This roughly corresponds to the implicit parameters to the current
event.notify_<is_additional>_<relaxed>(count)
function.is_additional
sets theadditional
property totrue
, andrelaxed
makes the fence do nothing instead of emitting aSeqCst
fence. Thenext_tag
function emits the tag that gets attached to the notification.I wanted to give this some power, so I created the IntoNotification trait as well.
This way, if we make
notify()
take animpl IntoNotification
instead ofusize
, it still works as intended, as the implementation forusize::into_notification
is non-additional and non-relaxed:You can also use this system to add combinators (
.additional()
,.relaxed()
), such that:Then, to add tags to a notification, you would do:
Discussion questions:
Notification
andIntoNotification
be sealed? If it is, it means we can add new methods to it without worrying. If it isn't, it means that users can implement their own notification types. However, that may cause behavior we don't expect.