More Clippy fixes for alloc, core and std #64178

mati865 · 2019-09-05T15:29:54Z

Continuation of #63805

rust-highfive · 2019-09-05T15:30:05Z

r? @TimNN

(rust_highfive has picked a reviewer for you, use r? to override)

src/libstd/thread/mod.rs

src/libcore/iter/adapters/mod.rs

src/libcore/iter/traits/iterator.rs

scottmcm · 2019-09-09T00:33:13Z

src/libcore/iter/traits/iterator.rs

@@ -2742,7 +2742,7 @@ pub trait Iterator {
            None => return true,
        };

-        while let Some(curr) = self.next() {
+        for curr in self {


For comparison to the previous comment, this one seems fine, since it can move self and doesn't add extra &muts. (Though it should arguably be rewritten as a try_fold...)

Thank you for explanation.
I cannot figure out how to change it to try_fold so should I keep it or revert?

I would revert.

IIRC generated assembly was exactly the same but I'll drop it anyway.
Should I add FIXME about rewriting it with try_fold?

src/libstd/sys/windows/handle.rs

Alexendoo · 2019-09-18T19:41:27Z

~~Ping from triage, any updates? @scottmcm~~

Alexendoo · 2019-10-02T13:53:03Z

Ping from triage, any updates? @TimNN

TimNN · 2019-10-05T17:04:37Z

Sorry, I don't have the time right now to review Rust PRs, so

r? @scottmcm

since they already commented on this PR.

JohnCSimon · 2019-10-12T05:19:57Z

Ping from triage
@scottmcm can you please review this?
Thanks

src/liballoc/borrow.rs

Mark-Simulacrum · 2019-10-17T23:35:10Z

src/libcore/iter/adapters/mod.rs

-            if self.iter.nth(to_skip-1).is_none() {
-                return None;
-            }
+            self.iter.nth(to_skip - 1).as_ref()?;


I think this should not be done in this PR. Using ? may have performance implications compared to the old code (as well as creating the reference -- which seems odd).

I honestly have no idea about this as_ref(), possibly it was added by IDE when I was applying lints.

? has been used 14 times in this whole file although it performs a bit worse: https://godbolt.org/z/nHS4fh
I'll drop it but what about other 14 uses in this file, should they be left as is or replaced?

Mark-Simulacrum · 2019-10-17T23:36:00Z

src/libcore/iter/traits/iterator.rs

@@ -1684,6 +1684,7 @@ pub trait Iterator {
    /// assert_eq!(it.len(), 2);
    /// assert_eq!(it.next(), Some(&40));
    /// ```
+    #[allow(clippy::while_let_on_iterator)] // uses mutable borrow resulting in worse assembly


I don't think we should add this to libstd, at least for now; I'm not sure libs team wants these scattered around.

Sure I'll remove all Clippy annotations.
One day if Clippy is expected to be over Rust codebase this will be probably unavoidable though. Probably topic for a meeting.

Also dropped mati865@694e625
Should I bring annotations topic to the libs teams somehow?

Mark-Simulacrum

A lot of these are fine but left a few comments.

Mark-Simulacrum · 2019-10-17T23:36:13Z

src/libcore/iter/traits/iterator.rs

@@ -2742,7 +2742,7 @@ pub trait Iterator {
            None => return true,
        };

-        while let Some(curr) = self.next() {
+        for curr in self {


I would revert.

src/libstd/thread/mod.rs

mati865 · 2019-10-19T17:28:44Z

Applied review comments.

Open questions:

scottmcm

This looks pretty good, I have just two requests before r=me

src/libstd/thread/mod.rs

src/libcore/option.rs

mati865 · 2019-10-21T16:18:35Z

Updated the code and the list of (my) open questions: #64178 (comment)

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

bors · 2019-10-22T15:44:04Z

☔ The latest upstream changes (presumably #65503) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2019-10-23T17:58:51Z

Thanks, @mati865!

@bors r+ rollup

bors · 2019-10-23T17:58:52Z

📌 Commit bedbf3b has been approved by scottmcm

More Clippy fixes for alloc, core and std Continuation of rust-lang#63805

@ghost

Rollup of 12 pull requests Successful merges: - #64178 (More Clippy fixes for alloc, core and std) - #65144 (Add Cow::is_borrowed and Cow::is_owned) - #65193 (Lockless LintStore) - #65479 (Add the `matches!( $expr, $pat ) -> bool` macro) - #65518 (Avoid ICE when checking `Destination` of `break` inside a closure) - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.) - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.) - #65648 (Eliminate `intersect_opt`.) - #65657 (Remove `InternedString`) - #65691 (Update E0659 error code long explanation to 2018 edition) - #65696 (Fix an issue with const inference variables sticking around under Chalk + NLL) - #65704 (relax ExactSizeIterator bound on write_bytes) Failed merges: r? @ghost

rust-highfive assigned TimNN Sep 5, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2019

mati865 commented Sep 5, 2019

View reviewed changes

src/libstd/thread/mod.rs Outdated Show resolved Hide resolved

tesuji reviewed Sep 5, 2019

View reviewed changes

src/libstd/thread/mod.rs Outdated Show resolved Hide resolved

jakubadamw reviewed Sep 5, 2019

View reviewed changes

src/libcore/iter/adapters/mod.rs Outdated Show resolved Hide resolved

scottmcm reviewed Sep 9, 2019

View reviewed changes

src/libcore/iter/traits/iterator.rs Outdated Show resolved Hide resolved

scottmcm reviewed Sep 9, 2019

View reviewed changes

src/libstd/sys/windows/handle.rs Show resolved Hide resolved

mati865 force-pushed the clippy branch from 4d4d5d5 to 4e2aa9b Compare September 9, 2019 11:58

mati865 force-pushed the clippy branch from 4e2aa9b to 9df5072 Compare September 24, 2019 13:45

rust-highfive assigned scottmcm and unassigned TimNN Oct 5, 2019

Mark-Simulacrum reviewed Oct 17, 2019

View reviewed changes

mati865 force-pushed the clippy branch from 9df5072 to 143ebc7 Compare October 19, 2019 17:10

scottmcm requested changes Oct 21, 2019

View reviewed changes

src/libstd/thread/mod.rs Outdated Show resolved Hide resolved

src/libcore/option.rs Outdated Show resolved Hide resolved

scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 21, 2019

mati865 force-pushed the clippy branch from 143ebc7 to c1b9d62 Compare October 21, 2019 16:13

mati865 added 3 commits October 22, 2019 19:14

Apply clippy::useless_let_if_seq suggestion

dffc1b3

Apply clippy::needless_return suggestions

95c06a2

Apply clippy::redundant_pattern_matching suggestion

e1aa297

mati865 added 2 commits October 22, 2019 19:23

Apply clippy::while_let_on_iterator suggestions

7491468

Apply clippy::single_match suggestion

bedbf3b

mati865 force-pushed the clippy branch from c1b9d62 to bedbf3b Compare October 22, 2019 17:24

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 22, 2019

mati865 requested a review from scottmcm October 23, 2019 17:43

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2019

Centril added a commit to Centril/rust that referenced this pull request Oct 23, 2019

Rollup merge of rust-lang#64178 - mati865:clippy, r=scottmcm

426c6cf

More Clippy fixes for alloc, core and std Continuation of rust-lang#63805

Centril mentioned this pull request Oct 23, 2019

Rollup of 12 pull requests #65733

Merged

bors merged commit bedbf3b into rust-lang:master Oct 24, 2019

mati865 deleted the clippy branch October 24, 2019 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More Clippy fixes for alloc, core and std #64178

More Clippy fixes for alloc, core and std #64178

mati865 commented Sep 5, 2019

rust-highfive commented Sep 5, 2019

scottmcm Sep 9, 2019

mati865 Sep 9, 2019

Mark-Simulacrum Oct 17, 2019

mati865 Oct 18, 2019

Alexendoo commented Sep 18, 2019 •

edited

Loading

Alexendoo commented Oct 2, 2019

TimNN commented Oct 5, 2019

JohnCSimon commented Oct 12, 2019

Mark-Simulacrum Oct 17, 2019

mati865 Oct 18, 2019

Mark-Simulacrum Oct 17, 2019

mati865 Oct 18, 2019

mati865 Oct 19, 2019 •

edited

Loading

Mark-Simulacrum left a comment

Mark-Simulacrum Oct 17, 2019

mati865 commented Oct 19, 2019 •

edited

Loading

scottmcm left a comment

mati865 commented Oct 21, 2019 •

edited

Loading

bors commented Oct 22, 2019

scottmcm commented Oct 23, 2019

bors commented Oct 23, 2019

More Clippy fixes for alloc, core and std #64178

More Clippy fixes for alloc, core and std #64178

Conversation

mati865 commented Sep 5, 2019

rust-highfive commented Sep 5, 2019

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

Alexendoo commented Sep 18, 2019 • edited Loading

Alexendoo commented Oct 2, 2019

TimNN commented Oct 5, 2019

JohnCSimon commented Oct 12, 2019

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

mati865 Oct 19, 2019 • edited Loading

Choose a reason for hiding this comment

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mati865 commented Oct 19, 2019 • edited Loading

scottmcm left a comment

Choose a reason for hiding this comment

mati865 commented Oct 21, 2019 • edited Loading

bors commented Oct 22, 2019

scottmcm commented Oct 23, 2019

bors commented Oct 23, 2019

Alexendoo commented Sep 18, 2019 •

edited

Loading

mati865 Oct 19, 2019 •

edited

Loading

mati865 commented Oct 19, 2019 •

edited

Loading

mati865 commented Oct 21, 2019 •

edited

Loading