-
Notifications
You must be signed in to change notification settings - Fork 668
Add private fields to public types for forward compatibility #1841
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
Conversation
9ee47f2 to
67eaf16
Compare
36a4a16 to
7df2688
Compare
|
|
||
| /// Error returned from a [`Receiver`](Receiver) when the corresponding | ||
| /// [`Sender`](Sender) is dropped. | ||
| #[derive(Clone, Copy, PartialEq, Eq, Debug)] |
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 actually like this one as-is because it allows you to easily write it as a pattern-- e.g.
match recv.await {
Ok(x) => ...,
Err(Canceled) => ...
}I can't really imagine what else we'd want to add to it in the future.
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.
Okay, these are in util/channel, so we can change them if we need them.
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.
FWIW, I tend to write it like:
match rx.await {
Err(_canceled) => (),
}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.
@seanmonstar That doesn't actually assert that cancelled is what happened there, though-- you could be silencing a real error by accident.
| } | ||
|
|
||
| /// Indicator that the `Abortable` future was aborted. | ||
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] |
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.
Same comment as with Canceled-- this is nice for matching on, and I can't forsee wanting to add anything here.
|
|
||
| drop(tx2); | ||
|
|
||
| assert_eq!(Poll::Ready(Some(Err(oneshot::Canceled))), queue.poll_next_unpin(cx)); |
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 the bad thing about this PR is that we cannot do this.
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.
However, actually, this is not just a problem for this PR. I also encountered this in #1785.
No description provided.