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

Some clippy fixes in the compiler #110124

Merged
merged 16 commits into from
Apr 10, 2023
Merged

Conversation

Nilstrieb
Copy link
Member

Best reviewed commit-by-commit 📎.

@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2023

r? @jackh726

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

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Apr 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented Apr 9, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @TaKO8Ki

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

r=me

compiler/rustc_session/src/parse.rs Show resolved Hide resolved
compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/traits/mod.rs Show resolved Hide resolved
compiler/rustc_macros/src/diagnostics/fluent.rs Outdated Show resolved Hide resolved
compiler/rustc_data_structures/src/profiling.rs Outdated Show resolved Hide resolved
The shadowing lead to an incorrect comparison. Rename it and compare it
properly. compiler-errors told me that I should just include the fix
here without a test.
Exhaustively destructure and ignore `()`
@Nilstrieb
Copy link
Member Author

I removed the Don't assume that io::{Read, Write} writes/reads all commit since that seems to be causing an ICE that I can't be bothered to further investigate, the code is fine as-is even though it technically relies on unspecified details of Read,Write.

@saethlin
Copy link
Member

saethlin commented Apr 9, 2023

How do we repro the ICE? Seems worth someone writing an issue

@Nilstrieb
Copy link
Member Author

I used read_exact but there aren't enough bytes in the data to fill the buffer, returning an error. read just returned as many bytes as there are, which is the desired behavior here. The code isn't wrong so it's fine to leave it, but it should maybe be rewritten in the future to not do it like that, not that it's very important.
a90bd02df386787c621cdfcc17b96a8e3ecb13e9

Co-authored-by: Michael Goulet <michael@errs.io>
@compiler-errors
Copy link
Member

Ok, looking at a90bd02, I think it makes sense that the read_all would fail -- we're trying to read at most a u128 from the source buffer, but not necessarily a u128, so source is not necessarily at least sizeof(u128) long. Anyways, looks fine now.

r? @compiler-errors @bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2023

📌 Commit 9fc1555 has been approved by compiler-errors

It is now in the queue for this repository.

@rustbot rustbot assigned compiler-errors and unassigned jackh726 Apr 10, 2023
@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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2023
@@ -5,7 +5,7 @@ const RED_ZONE: usize = 100 * 1024; // 100k

// Only the first stack that is pushed, grows exponentially (2^n * STACK_PER_RECURSION) from then
// on. This flag has performance relevant characteristics. Don't set it too high.
const STACK_PER_RECURSION: usize = 1 * 1024 * 1024; // 1MB
const STACK_PER_RECURSION: usize = 1024 * 1024; // 1MB
Copy link
Member

Choose a reason for hiding this comment

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

TBH, I think clippy is wrong to complain about this. Especially in a const, there's absolutely nothing wrong with a 1 *, and representing "X MB" as X * 1024 * 1024 is fine.

Maybe use 1 << 20? That's also clearly 1 MiB, and hopefully clippy wouldn't whine about it.

Copy link
Member

Choose a reason for hiding this comment

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

I wish I agreed 1 << 20 was obviously 1 MB. Maybe I'm just uncommonly bad at all these bit twiddling operations.

Copy link
Member

@scottmcm scottmcm Apr 10, 2023

Choose a reason for hiding this comment

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

(For context: k is a decimal shift by 3, M is a decimal shift by 6, etc. The elementary school "move the decimal point" is, essentially, a shift. So M ≡ 10⁶, aka "<<₁₀ 6". And similarly Mi ≡ 2²⁰, aka << 20. So "kebi", "mebi", "gibi", "tebi", etc are "<< 10", "<< 20", "<< 30", "<< 40", etc.)

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#109724 (prioritize param env candidates if they don't guide type inference)
 - rust-lang#110021 (Fix a couple ICEs in the new `CastKind::Transmute` code)
 - rust-lang#110044 (Avoid some manual slice length calculation)
 - rust-lang#110115 (compiletest: Use remap-path-prefix only in CI)
 - rust-lang#110121 (Fix `x check --stage 1` when download-rustc is enabled)
 - rust-lang#110124 (Some clippy fixes in the compiler)

Failed merges:

 - rust-lang#109752 (Stall auto trait assembly in new solver for int/float vars)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 97921ab into rust-lang:master Apr 10, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 10, 2023
@Nilstrieb Nilstrieb deleted the 📎-told-me-so branch April 10, 2023 11:50
@lcnr lcnr removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.

None yet

8 participants