Skip to content

Some let chains clean-up #144469

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Kivooeo
Copy link
Contributor

@Kivooeo Kivooeo commented Jul 25, 2025

Not sure if this kind of clean-up is welcoming because of size, but I decided to try out one

r? compiler

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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 Jul 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2025

Some changes occurred to constck

cc @fee1-dead

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

HIR ty lowering was modified

cc @fmease

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) C-cleanup Category: PRs that clean code up or issues documenting cleanup. and removed A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) labels Jul 25, 2025
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) labels Jul 25, 2025
@SparrowLii
Copy link
Member

SparrowLii commented Jul 26, 2025

Thanks for your work! Could you split the change to a few commits to make it easier for review(and for merging)?

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Jul 26, 2025

Well, I've found some time, did you mean to split commits like this (I'll reword them later if needed)? Or like... different PRs?

@SparrowLii
Copy link
Member

SparrowLii commented Jul 26, 2025

One PR is enough. But TBH I don't see some real benefit from doing these clean-ups, since they don't significantly reduce the number of lines of code. And it may change the logic of the original design. For example, developers may add some else statements in the future

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Jul 26, 2025

Originally idea come up from this two

66ea349

ccbe983

and such other PRs that I saw before from different people that refactor few places to use let chains instead of nesting

@SparrowLii
Copy link
Member

SparrowLii commented Jul 26, 2025

OK. From this point of view, instead of thinking "why do this", maybe it is better to answer "why not do this" :)
I still advice you separate this PR into several commits(according to the distribution of code functionality). After all, we don't have an automated tool to check whether the conversion from nesting to let-chain is correct. This makes it easier to locate problems (and review)

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Jul 26, 2025

@SparrowLii I'm pretty sure I've splitted this PR already into separate commits, this one is 3 commits instead of 1. If you meant something different by splitting PR please could you explain then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants