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.

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

Improved code with clippy #124287

Merged
merged 1 commit into from
Apr 25, 2024
Merged

Improved code with clippy #124287

merged 1 commit into from
Apr 25, 2024

Conversation

41Leahcim
Copy link
Contributor

I haven't used the bootstrapped compiler, but I think I have made some improvements using clippy. I have already made the following changes to the compiler:
Replaced self.first().is_digit(10) with self.first().is_ascii_digit() on lines 633, 664, and 680 of compiler/rust_lexer/src/lib.rs.

Removed unnecessary cast on line 262 of compiler/rustc_lexer/src/unescape.rs

Replaced ok_or_else with ok_or on line 303 of compiler/rustc_lexer/src/unescape.rs

Replaced !std::env::var("RUSTC_BOOTSTRAP").is_ok() with std::env::var("RUSTC_BOOTSTRAP").is_err() on line 4 of compiler/rustc_macros/build.rs

Removed needless borrow for generic argument envon line 53 of compiler/rust_llvm/build.rs

@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@41Leahcim
Copy link
Contributor Author

II have also changed some tooling code with clippy. I have made the following changes:
Turned the argument slice to an array on line 33 of src/tools/build_helper/src/ci.rs

Removed question mark operator on line 24 of src/tools/build_helper/src/git.rs

Removed a needless borrow implicitly performed by the compiler on line 147 of src/tools/clippy/declare_clippy_lint/src/lib.rs

Removed a needless borrow on line 54 of src/tools/miropt-test-tools/src/lib.rs

Replaced match expression with matches! macro on line 54 and 61 of src/tool/rustfmt/config_proc_macro/src/attrs.rs

Removed a needless borrow on lines 78 and 153 of src/tools/rustfmt/config_proc_macro/src/item_enum.rs

Replaced match expression with an equality check on line 16 of src/tools/rustfmt/config_proc_macro/src/utils.rs

Replaced argument slices to arrays on lines 43 and 51 of src/tools/rustfmt/build.rs

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

The rustfmt team prefers these kinds of changes be made directly to rustfmt instead of here in the rust-lang/rust repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert these changes and open a PR directly to https://github.com/rust-lang/rustfmt for these kinds of changes.

Copy link
Member

Choose a reason for hiding this comment

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

(where "these changes" means anything in clippy or rustfmt)

Copy link
Contributor

@ytmimi ytmimi Apr 23, 2024

Choose a reason for hiding this comment

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

@41Leahcim to clarify, some of the tools in rust-lang/rust are developed in their own repos, and it's best to avoid making non essential, lint related changes here. Instead, prefer to open PRs directly to the respective repos.

Please revert all changes made to src/tools/rustfmt and src/tools/clippy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for telling me. I'm pretty new to open source. That's why I'm still making stupid mistakes like this and why my pull request doesn't add anything special.

@41Leahcim
Copy link
Contributor Author

I have reverted the changes to all tools. Only the changes to the compiler are left. With only 7 lines changed, it's a pretty much worthless pull request. So I would understand it if it won't get merged.

@fmease
Copy link
Member

fmease commented Apr 23, 2024

It's alright. I will approve it once you've squashed the commits into one.
r? fmease

@rustbot rustbot assigned fmease and unassigned compiler-errors Apr 23, 2024
@fmease
Copy link
Member

fmease commented Apr 23, 2024

Note that your two long comments describing the changes in prose are now outdated.
Friendly tip: No need to transliterate code changes to prose, paraphrasing and summarizing does the job a lot better.

@fmease fmease 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 Apr 23, 2024
@41Leahcim
Copy link
Contributor Author

I have merged my commits

@fmease
Copy link
Member

fmease commented Apr 25, 2024

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 25, 2024

📌 Commit 3c5e88c has been approved by fmease

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2024
…iaskrgr

Rollup of 2 pull requests

Successful merges:

 - rust-lang#124287 (Improved code with clippy)
 - rust-lang#124326 (tests: remove few ignore-stage2)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b28721f into rust-lang:master Apr 25, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 25, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2024
Rollup merge of rust-lang#124287 - 41Leahcim:master, r=fmease

Improved code with clippy

I haven't used the bootstrapped compiler, but I think I have made some improvements using clippy. I have already made the following changes to the compiler:
Replaced `self.first().is_digit(10)` with `self.first().is_ascii_digit()` on lines 633, 664, and 680 of compiler/rust_lexer/src/lib.rs.

Removed unnecessary cast on line 262 of compiler/rustc_lexer/src/unescape.rs

Replaced ok_or_else with ok_or on line 303 of compiler/rustc_lexer/src/unescape.rs

Replaced `!std::env::var("RUSTC_BOOTSTRAP").is_ok()` with `std::env::var("RUSTC_BOOTSTRAP").is_err()` on line 4 of compiler/rustc_macros/build.rs

Removed needless borrow for generic argument `env`on line 53 of compiler/rust_llvm/build.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants