-
-
Notifications
You must be signed in to change notification settings - Fork 25
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(mpsc): add len
, capacity
, and remaining
methods to mpsc
#72
Conversation
len
, capacity
, and remaining
methods to mpsc (#71)len
, capacity
, and remaining
methods to mpsc
34b0e9e
to
6b7f22f
Compare
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.
overall, this looks good to me, thanks! i commented on a few minor suggestions inline.
also, I notice that this branch only adds these methods to the mpsc::Sender
and mpsc::Receiver
types. can we also add similar methods to the other channel flavors:
mpsc::StaticSender
:
thingbuf/src/mpsc/async_impl.rs
Line 911 in 6b7f22f
impl<T, R> StaticSender<T, R> mpsc::StaticReceiver
:
thingbuf/src/mpsc/async_impl.rs
Line 1133 in 6b7f22f
impl<T, R> StaticReceiver<T, R> { mpsc::blocking::StaticSender
:Line 244 in 6b7f22f
impl<T, R> StaticSender<T, R> mpsc::blocking::StaticReceiver
:Line 452 in 6b7f22f
impl<T, R> StaticReceiver<T, R> { mpsc::blocking::Sender
:Line 716 in 6b7f22f
impl<T, R> Sender<T, R> mpsc::blocking::Receiver
:Line 915 in 6b7f22f
impl<T, R> Receiver<T, R> {
i realize this is a bunch of additional code to add, but for the most part, it should be possible to copy and paste the code from mpsc/async_impl.rs
to the other files, with minor changes to the doctests to import and construct the appropriate types.
thank you!
src/mpsc/async_impl.rs
Outdated
/// | ||
/// [`remaining`]: Self::remaining | ||
#[inline] | ||
pub fn capacity(&self ) -> usize { |
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.
tiny style nit:
pub fn capacity(&self ) -> usize { | |
pub fn capacity(&self) -> usize { |
src/mpsc/async_impl.rs
Outdated
/// | ||
/// [`remaining`]: Self::remaining | ||
#[inline] | ||
pub fn capacity(&self ) -> usize { |
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.
pub fn capacity(&self ) -> usize { | |
pub fn capacity(&self) -> usize { |
src/mpsc/async_impl.rs
Outdated
/// | ||
/// [`remaining`]: Self::remaining | ||
#[inline] | ||
#[allow(clippy::len_without_is_empty)] |
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.
i think it would be fine to also add an is_empty
method :)
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.
and also
#[allow(clippy::len_without_is_empty)] | |
#[allow(clippy::len_without_is_empty)] | |
#[must_use] |
src/mpsc/async_impl.rs
Outdated
/// | ||
/// [`remaining`]: Self::remaining | ||
#[inline] | ||
#[allow(clippy::len_without_is_empty)] |
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.
as above, i think we can just add an is_empty
method to this as well.
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.
and
#[allow(clippy::len_without_is_empty)] | |
#[allow(clippy::len_without_is_empty)] | |
#[must_use] |
Ah, didn't notice that there was a |
Great, thank you! By the way, PR #73 fixed the build failure due to Clippy lints, so you may want to rebase onto the latest Thanks again! |
659147a
to
6b5e0c3
Compare
6b5e0c3
to
cfd9439
Compare
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.
this looks good to me, thank you!
one last docs nit: it looks like the documentation for the StaticReceiver
/StaticSender
methods links to the non-static Sender
/Receiver
types. i'm going to just go ahead and fix that, and merge this branch.
thanks again for the PR!
ugh, i seem to have messed something up, hang on... |
these tests drop the rx, closing the channel. thus, the test fails.
## v0.1.5 (2024-04-06) #### Features * **mpsc:** add `len`, `capacity`, and `remaining` methods to mpsc (#72) ([00213c1](00213c1), closes [#71](#71)) #### Bug Fixes * unused import with `alloc` enabled ([ac1eafc](ac1eafc)) * skip slots with active reading `Ref`s in `push_ref` (#81) ([a72a286](a72a286), closes [#83](#83), [#80](#80))
Fixes #71