Conversation
k-nasa
left a comment
k-nasa
left a comment
There was a problem hiding this comment.
Thanks for PR.
This PR has many changes, so it takes time to confirm.
I think we can merge quickly only changing the documentation.
Can you split PR into changing documentation and refactoring?
|
Needs a rebase. |
|
Is it good now @Fishrock123 ? |
Fishrock123
left a comment
Fishrock123
left a comment
There was a problem hiding this comment.
All the consts worry me, I don't know enough what implications those have.
There are some other changes that should be made or undone.
| /// ``` | ||
| pub fn new() -> DirBuilder { | ||
| #[must_use] | ||
| pub const fn new() -> DirBuilder { |
There was a problem hiding this comment.
What is the benefit of making this const?
There was a problem hiding this comment.
The benefit of making this const is that DirBuilder::new can now be used in const declarations, as well as anything that depends on it also now has a chance to be made const.
| impl ReadDir { | ||
| /// Creates an asynchronous `ReadDir` from a synchronous handle. | ||
| pub(crate) fn new(inner: std::fs::ReadDir) -> ReadDir { | ||
| pub(crate) const fn new(inner: std::fs::ReadDir) -> ReadDir { |
There was a problem hiding this comment.
Can this even be const? Even inner will be different, how would it generate code to account for that?
There was a problem hiding this comment.
I'm not sure I understand your reasoning here
| fn test_read_by_ref() { | ||
| crate::task::block_on(async { | ||
| let mut f = io::Cursor::new(vec![0u8, 1, 2, 3, 4, 5, 6, 7, 8]); | ||
| let mut f = io::Cursor::new(vec![0_u8, 1, 2, 3, 4, 5, 6, 7, 8]); |
There was a problem hiding this comment.
Just more readable, and idiomatic/conventional to have an underscore in Rust. I agree that its not strictly necessary at all
| match this.future.poll(cx) { | ||
| Poll::Pending => {} | ||
| other => return other, | ||
| other @ Poll::Ready(..) => return other, |
There was a problem hiding this comment.
Unnecessary, if Poll were to expand there would be issues anyways and that would almost certainly have to happen in a rust edition.
There was a problem hiding this comment.
I think the goal here is to make clearer to readers that the only possibility is Poll::Ready(..), rather than being unclear about the possibly multiple cases it covers.
| /// | ||
| /// [`ancestors`]: struct.Path.html#method.ancestors | ||
| /// [`Path`]: struct.Path.html | ||
| #[derive(Copy, Clone, Debug)] |
There was a problem hiding this comment.
This is a breaking change. What is the reasoning?
There was a problem hiding this comment.
I think this might have been a mistake, I'll revert.
| this.source.poll_next(cx) | ||
| } | ||
| item => Poll::Ready(item), | ||
| item @ Some(..) => Poll::Ready(item), |
There was a problem hiding this comment.
I think the goal here is to make clearer to readers that the only possibility is Some(..), rather than being unclear about the possibly multiple cases it covers.
| Some(v) => match (this.f)(v) { | ||
| Some(b) => Poll::Ready(Some(b)), | ||
| None => { | ||
| Some(v) => (this.f)(v).map_or_else( |
There was a problem hiding this comment.
Was this clippy? I think this is harder to read.
There was a problem hiding this comment.
This was clippy's suggestion (or after a series of suggestions), removing unnecessary duplication
| Some(v) => match (&mut self.f)(v) { | ||
| Some(v) => Poll::Ready(Some(v)), | ||
| None => { | ||
| Some(v) => (&mut self.f)(v).map_or_else( |
| let lock = RwLock::new(()); | ||
| drop(lock.read().await); | ||
| drop(lock.write().await); | ||
| drop((lock.read().await, lock.read().await)); |
There was a problem hiding this comment.
This is behaviorally different than the change. Please undo this. This will wait until both have read access before dropping them.
There was a problem hiding this comment.
Are you sure about this? clippy said it was a useless call, so it might be a clippy bug if you're right
There was a problem hiding this comment.
I am absolutely sure.
(lock.read().await, lock.read().await) means two immutable read locks are acquired and simultaneously alive before drop.
Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>
|
I've run |
4 participants
Fixes some clippy warnings like adding
constand#[must_use]where we can, putting Rust identifiers in tick marks in the documentation, etc.This may or may not involve breaking changes, so feel free to reject this if it is outside of scope for one PR or not good for right now.