Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolve some Clippy warnings #5835

Merged
merged 9 commits into from
Jul 31, 2018
Merged

Resolve some Clippy warnings #5835

merged 9 commits into from
Jul 31, 2018

Conversation

dwijnand
Copy link
Member

I'm not sure how these popped up since my PR 8 days ago.

My current hypotheses:

  • changes in latest nightly rust/clippy
  • new or changed cargo code
  • I missed these as I was only touching src/bin/cargo/main.rs

For future reference I now iterate with:

touch src/bin/cargo/main.rs src/cargo/lib.rs && cargo +nightly clippy

dwijnand added 2 commits July 31, 2018 09:58
@dwijnand
Exempt 3 identity_op lint warnings
4500704
@dwijnand
Exempt too_many_arguments throughout the cargo lib crate
9c3f808 
I'm confused.  Now this is duplicated with src/bin/cargo/main.rs and I
don't know how to avoid that.  Tips welcome.
@rust-highfive
Copy link

r? @matklad

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

@dwijnand
Copy link
Member Author

Feedback welcome 🙂

@alexcrichton
Copy link
Member

FWIW I'm personally not a huge fan of #[cfg] annotations all through for various clippy lints, it's sort of the exact reason why I'm not a fan of clippy in general (it's too overzealous in linting). This certainly isn't specific to this PR though! In general I'm not a huge fan of clippy, so nothing against this PR itself :)

Is there perhaps a different location (not in the source code) that lints could be turned off?

@dwijnand
Copy link
Member Author

Maybe after #5728 is merged and we're dogfooding that? Though that would only work for the ones we're happy to toggle "globally".

dwijnand added 6 commits July 31, 2018 15:12
@dwijnand
Resolve 2 needless_pass_by_value lint warnings
689f412 
TIL Rust doesn't have equational reasoning.. :-/
(can't inline the new "exec" bindings)
@dwijnand
Resolve a wrong_self_convention lint warning
1848645 
.. by inlining & removing `Members::is_empty`.

For details see
https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#wrong_self_convention

Asides from opt-ing out, the alternative I saw was calling it
"into_empty" and make it return a little

    enum Empty { Empty, NonEmpty }

type.
@dwijnand
Resolve a wrong_self_convention lint warning
8947ed1
@dwijnand
Resolve a trivially_copy_pass_by_ref lint warning
a0a1ad6
@dwijnand
Exempt 2 wrong_self_convention lint warnings
a661b33
@dwijnand
Resolve a single_match lint warning
e55ecf9
@dwijnand
Copy link
Member Author

Fixed up a commit where I'd forgotten to adapt the test sources too.

@dwijnand
Copy link
Member Author

Now that I think of it, @alexcrichton, clippy configuration is also being discussed in the Clippy 1.0 RFC (rust-lang/rfcs#2476), if you want to get stuck in. 😄

@alexcrichton
Copy link
Member

Perhaps for now the lints could all be applied globally in src/cargo/lib.rs and grouped there? It will turn off unnecessarily in some circumstances but I think that's fine.

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Jul 31, 2018

📌 Commit 0807730 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 31, 2018

⌛ Testing commit 0807730 with merge 72eee12...

bors added a commit that referenced this pull request Jul 31, 2018
@bors
Auto merge of #5835 - dwijnand:clippy, r=alexcrichton
72eee12 
Resolve some Clippy warnings

I'm not sure how these popped up since my PR 8 days ago.

My current hypotheses:
* changes in latest nightly rust/clippy
* new or changed cargo code
* I missed these as I was only touching `src/bin/cargo/main.rs`

For future reference I now iterate with:

    touch src/bin/cargo/main.rs src/cargo/lib.rs && cargo +nightly clippy
@bors
Copy link
Contributor

bors commented Jul 31, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 72eee12 to master...

@bors bors merged commit 0807730 into rust-lang:master Jul 31, 2018
@dwijnand dwijnand deleted the clippy branch August 1, 2018 05:49
@ehuss ehuss added this to the 1.29.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@matklad matklad

Labels
None yet
Projects
None yet
Milestone
1.29.0
Development

Successfully merging this pull request may close these issues.
6 participants
@dwijnand @rust-highfive @alexcrichton @bors @ehuss @matklad