-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add Stream wrappers in tokio-stream #3343
Conversation
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 implementation LGTM.
However, I'm not sure if it's better to put everything in a single module (wrappers
) or in multiple modules per feature (net
, io
, fs
). What do you think?
I think the main place where we risk name overlaps is with receivers of various types, but those are all in sync. |
@Darksonn the |
Yes, but I forgot to make them public, so we need a PR for that first. |
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.
LGTM
} | ||
} | ||
|
||
impl AsMut<Interval> for IntervalStream { |
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.
Just checking are there any issues exposing a mut reference while this stream is pinned?
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.
It's impossible to get an &mut S
after having pinned S
unless you use unsafe (or if S: Unpin
of course), so you can't call this method if you have pinned it.
Though arguably we may want to add an as_pin_mut
for non-Unpin
wrappers.
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.
good point, this should be fine for now then. I was thinking about the wrong thing, we are not doing any special unpinning here so we can expose the &mut T
This PR builds on #3342. When that PR is merged, the diff should become more easily readable.In this PR I add wrappers for every Tokio type that previously implemented
Stream
and has an exposed poll method. Notably, the poll method is missing onSignal
, so that type is not included in this PR.