Fix some clippy warnings #948

brightly-salty · 2021-01-31T19:52:28Z

Fixes some clippy warnings like adding const and #[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.

k-nasa

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?

Fishrock123 · 2021-05-31T19:25:28Z

Needs a rebase.

brightly-salty · 2021-06-01T16:12:14Z

Is it good now @Fishrock123 ?

Fishrock123

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.

Fishrock123 · 2021-06-01T19:07:11Z

src/fs/dir_builder.rs

@@ -36,7 +36,8 @@ impl DirBuilder {
    ///
    /// let builder = DirBuilder::new();
    /// ```
-    pub fn new() -> DirBuilder {
+    #[must_use]
+    pub const fn new() -> DirBuilder {


What is the benefit of making this const?

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.

Fishrock123 · 2021-06-01T19:07:42Z

src/fs/read_dir.rs

@@ -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) const fn new(inner: std::fs::ReadDir) -> ReadDir {


Can this even be const? Even inner will be different, how would it generate code to account for that?

I'm not sure I understand your reasoning here

Fishrock123 · 2021-06-01T19:08:58Z

src/io/read/mod.rs

@@ -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]);


This seems like churn...

Just more readable, and idiomatic/conventional to have an underscore in Rust. I agree that its not strictly necessary at all

Fishrock123 · 2021-06-01T19:10:56Z

src/io/timeout.rs

@@ -67,7 +67,7 @@ where
        let this = self.project();
        match this.future.poll(cx) {
            Poll::Pending => {}
-            other => return other,
+            other @ Poll::Ready(..) => return other,


Unnecessary, if Poll were to expand there would be issues anyways and that would almost certainly have to happen in a rust edition.

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.

Fishrock123 · 2021-06-01T19:11:26Z

src/path/ancestors.rs

@@ -21,7 +21,7 @@ use crate::path::Path;
 ///
 /// [`ancestors`]: struct.Path.html#method.ancestors
 /// [`Path`]: struct.Path.html
-#[derive(Copy, Clone, Debug)]


This is a breaking change. What is the reasoning?

I think this might have been a mistake, I'll revert.

src/path/iter.rs

Fishrock123 · 2021-06-01T19:14:12Z

src/stream/stream/cycle.rs

@@ -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),


Unnecessary

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.

Fishrock123 · 2021-06-01T19:15:32Z

src/stream/stream/filter_map.rs

-            Some(v) => match (this.f)(v) {
-                Some(b) => Poll::Ready(Some(b)),
-                None => {
+            Some(v) => (this.f)(v).map_or_else(


Was this clippy? I think this is harder to read.

This was clippy's suggestion (or after a series of suggestions), removing unnecessary duplication

Fishrock123 · 2021-06-01T19:15:44Z

src/stream/stream/find_map.rs

-            Some(v) => match (&mut self.f)(v) {
-                Some(v) => Poll::Ready(Some(v)),
-                None => {
+            Some(v) => (&mut self.f)(v).map_or_else(


Fishrock123 · 2021-06-01T19:17:32Z

tests/rwlock.rs

@@ -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));


This is behaviorally different than the change. Please undo this. This will wait until both have read access before dropping them.

Are you sure about this? clippy said it was a useless call, so it might be a clippy bug if you're right

I am absolutely sure.

(lock.read().await, lock.read().await) means two immutable read locks are acquired and simultaneously alive before drop.

Okay, I'll revert this

Co-authored-by: Jeremiah Senkpiel <[email protected]>

jayvdb · 2024-09-10T22:47:10Z

I've run cargo clippy and afaics all of the changes in this PR are no longer warnings.

Fix some clippy warnings

03d1910

k-nasa reviewed Feb 1, 2021

View reviewed changes

Merge branch 'master' into warnings

ca7f5b6

Fishrock123 reviewed Jun 1, 2021

View reviewed changes

Update src/path/iter.rs

5e30d86

Co-authored-by: Jeremiah Senkpiel <[email protected]>

jayvdb mentioned this pull request Sep 10, 2024

Apply clippy fixes #1090

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some clippy warnings #948

Fix some clippy warnings #948

brightly-salty commented Jan 31, 2021

k-nasa left a comment

Fishrock123 commented May 31, 2021

brightly-salty commented Jun 1, 2021

Fishrock123 left a comment

Fishrock123 Jun 1, 2021

brightly-salty Jun 1, 2021

Fishrock123 Jun 1, 2021

brightly-salty Jun 1, 2021

Fishrock123 Jun 1, 2021

brightly-salty Jun 1, 2021

Fishrock123 Jun 1, 2021

brightly-salty Jun 3, 2021

Fishrock123 Jun 1, 2021

brightly-salty Jun 3, 2021

Fishrock123 Jun 1, 2021

brightly-salty Jun 3, 2021

Fishrock123 Jun 1, 2021

brightly-salty Jun 1, 2021

Fishrock123 Jun 1, 2021

Fishrock123 Jun 1, 2021

brightly-salty Jun 1, 2021

Fishrock123 Jun 2, 2021

brightly-salty Jun 3, 2021

jayvdb commented Sep 10, 2024

Fix some clippy warnings #948

Are you sure you want to change the base?

Fix some clippy warnings #948

Conversation

brightly-salty commented Jan 31, 2021

k-nasa left a comment

Choose a reason for hiding this comment

Fishrock123 commented May 31, 2021

brightly-salty commented Jun 1, 2021

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented Sep 10, 2024