Skip to content

More Clippy fixes for alloc, core and std - #64178

Merged
bors merged 5 commits into
rust-lang:masterfrom
mati865:clippy
Oct 24, 2019
Merged

More Clippy fixes for alloc, core and std#64178
bors merged 5 commits into
rust-lang:masterfrom
mati865:clippy

Conversation

@mati865

@mati865mati865 commented Sep 5, 2019

Copy link
Copy Markdown
Member

Continuation of #63805

@rust-highfive

rust-highfive commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

r? @TimNN

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2019
mati865
mati865 commented Sep 5, 2019
View reviewed changes
Comment threadsrc/libstd/thread/mod.rs Outdated
tesuji
tesuji reviewed Sep 5, 2019
View reviewed changes
Comment threadsrc/libstd/thread/mod.rs Outdated
jakubadamw
jakubadamw reviewed Sep 5, 2019
View reviewed changes
Comment threadsrc/libcore/iter/adapters/mod.rs Outdated
scottmcm
scottmcm reviewed Sep 9, 2019
View reviewed changes
Comment threadsrc/libcore/iter/traits/iterator.rs Outdated
scottmcm
scottmcm reviewed Sep 9, 2019
View reviewed changes
Comment threadsrc/libcore/iter/traits/iterator.rs Outdated

@scottmcmscottmcmSep 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

@mati865mati865Sep 9, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Mark-SimulacrumMark-SimulacrumOct 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would revert.

@mati865mati865Oct 18, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

scottmcm
scottmcm reviewed Sep 9, 2019
View reviewed changes
Comment threadsrc/libstd/sys/windows/handle.rs Outdated
@Alexendoo

Alexendoo commented Sep 18, 2019•
edited
Loading

Copy link
Copy Markdown
Member

Ping from triage, any updates? @scottmcm

@Alexendoo

Alexendoo commented Oct 2, 2019

Copy link
Copy Markdown
Member

Ping from triage, any updates? @TimNN

@TimNN

TimNN commented Oct 5, 2019

Copy link
Copy Markdown
Contributor

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

r? @scottmcm

since they already commented on this PR.

@rust-highfiverust-highfive assigned scottmcm and unassigned TimNNOct 5, 2019
@JohnCSimon

JohnCSimon commented Oct 12, 2019

Copy link
Copy Markdown

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

Mark-Simulacrum
Mark-Simulacrum reviewed Oct 17, 2019
View reviewed changes
Comment threadsrc/liballoc/borrow.rs Outdated
Comment threadsrc/libcore/iter/adapters/mod.rs Outdated

@Mark-SimulacrumMark-SimulacrumOct 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@mati865mati865Oct 18, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment threadsrc/libcore/iter/traits/iterator.rs Outdated

@Mark-SimulacrumMark-SimulacrumOct 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mati865mati865Oct 18, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mati865mati865Oct 19, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Mark-Simulacrum
Mark-Simulacrum reviewed Oct 17, 2019
View reviewed changes

@Mark-SimulacrumMark-Simulacrum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadsrc/libcore/iter/traits/iterator.rs Outdated

@Mark-SimulacrumMark-SimulacrumOct 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would revert.

Comment threadsrc/libstd/thread/mod.rs Outdated
@mati865

mati865 commented Oct 19, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Applied review comments.

Open questions:

scottmcm
scottmcm requested changes Oct 21, 2019
View reviewed changes

@scottmcmscottmcm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadsrc/libstd/thread/mod.rs Outdated
Comment threadsrc/libcore/option.rs Outdated
@scottmcmscottmcm 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

mati865 commented Oct 21, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

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

@bors

bors commented Oct 22, 2019

Copy link
Copy Markdown
Collaborator

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

@rustbotrustbot 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
mati865 requested a review from scottmcmOctober 23, 2019 17:43
@scottmcm

scottmcm commented Oct 23, 2019

Copy link
Copy Markdown
Member

Thanks, @mati865!

@bors r+ rollup

@bors

bors commented Oct 23, 2019

Copy link
Copy Markdown
Collaborator

📌 Commit bedbf3b has been approved by scottmcm

@borsbors 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
@Centril
Rollup merge ofrust-lang#64178- mati865:clippy, r=scottmcm
426c6cf
More Clippy fixes for alloc, core and std
Continuation of rust-lang#63805
@CentrilCentril mentioned this pull request Oct 23, 2019
Merged
bors added a commit that referenced this pull request Oct 24, 2019
@bors
Auto merge of#65733- Centril:rollup-0zth66f, r=Centril
55e0063
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
@bors
bors merged commit bedbf3b into rust-lang:masterOct 24, 2019
@mati865
mati865 deleted the clippy branch October 24, 2019 07:48
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-SimulacrumMark-SimulacrumMark-Simulacrum left review comments
@scottmcmscottmcmAwaiting requested review from scottmcm
+2 more reviewers
@jakubadamwjakubadamwjakubadamw left review comments
@tesujitesujitesuji left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@scottmcmscottmcm

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

11 participants

@mati865@rust-highfive@Alexendoo@TimNN@JohnCSimon@bors@scottmcm@Mark-Simulacrum@jakubadamw@tesuji@rustbot