- Notifications
You must be signed in to change notification settings - Fork341
Fix some clippy warnings#948
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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.
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 ? |
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.
All theconsts worry me, I don't know enough what implications those have.
There are some other changes that should be made or undone.
| @@ -36,7 +36,8 @@ impl DirBuilder { | |||
| /// | |||
| /// let builder = DirBuilder::new(); | |||
| /// ``` | |||
| pub fn new() -> DirBuilder { | |||
| #[must_use] | |||
| pub const fn new() -> DirBuilder { | |||
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.
What is the benefit of making this const?
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 benefit of making this const is thatDirBuilder::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.
| @@ -80,7 +80,7 @@ enum State { | |||
| impl ReadDir { | |||
| /// Creates an asynchronous `ReadDir` from a synchronous handle. | |||
| pub(crate) fn new(inner: std::fs::ReadDir) -> ReadDir { | |||
| pub(crate)constfn new(inner: std::fs::ReadDir) -> ReadDir { | |||
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.
Can this evenbe const? Eveninner will be different, how would it generate code to account for that?
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'm not sure I understand your reasoning here
| @@ -485,7 +485,7 @@ mod tests { | |||
| #[test] | |||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems like churn...
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 more readable, and idiomatic/conventional to have an underscore in Rust. I agree that its not strictly necessary at all
| @@ -67,7 +67,7 @@ where | |||
| let this = self.project(); | |||
| match this.future.poll(cx) { | |||
| Poll::Pending => {} | |||
| other => return other, | |||
| other@ Poll::Ready(..)=> return other, | |||
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.
Unnecessary, ifPoll 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the goal here is to make clearer to readers that the only possibility isPoll::Ready(..), rather than being unclear about the possibly multiple cases it covers.
| @@ -21,7 +21,7 @@ use crate::path::Path; | |||
| /// | |||
| /// [`ancestors`]: struct.Path.html#method.ancestors | |||
| /// [`Path`]: struct.Path.html | |||
| #[derive(Copy, Clone, 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.
This is a breaking change. What is the reasoning?
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 this might have been a mistake, I'll revert.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -42,7 +42,7 @@ where | |||
| this.source.set(this.orig.clone()); | |||
| this.source.poll_next(cx) | |||
| } | |||
| item => Poll::Ready(item), | |||
| item@ Some(..)=> Poll::Ready(item), | |||
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.
Unnecessary
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 goal here is to make clearer to readers that the only possibility isSome(..), 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Was this clippy? I think this is harder to read.
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 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( |
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
| @@ -49,7 +49,8 @@ fn smoke() { | |||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am absolutely sure.
(lock.read().await, lock.read().await) means two immutableread locks are acquired and simultaneously alive before drop.
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, I'll revert this
Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>
I've run |
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.