Extract an arguments struct for `Builder::then_else_break` by Zalathar · Pull Request #121928 · rust-lang/rust

Zalathar · 2024-03-03T04:33:22Z

Most of this method's arguments are usually or always forwarded as-is to recursive invocations.

Wrapping them in a dedicated struct allows us to document each struct field, and lets us use struct-update syntax to indicate which arguments are being modified when making a recursive call.

While trying to understand the lowering of if expressions, I found it difficult to keep track of the half-dozen arguments passed through to every call to then_else_break. I tried switching over to an arguments struct, and I found that it really helps to make sense of what each argument does, and how each call is modifying the arguments.

I have some further ideas for how to streamline these recursive calls, but I've kept those out of this PR so that it's a pure refactoring with no behavioural changes.

rustbot · 2024-03-03T04:33:29Z

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-03-03T04:33:31Z

Some changes occurred in match lowering

cc @Nadrieril

Zalathar · 2024-03-03T04:34:15Z

All of the new comments are based on my own understanding, so they could certainly use a second opinion from someone more familiar with this code.

Nadrieril · 2024-03-03T12:27:11Z

I have one comment, otherwise I think this is a net improvement in code legibility, thank you!

r? @Nadrieril

Zalathar · 2024-03-04T02:27:00Z

Updated to address feedback (diff).

Now all of the external callers are unchanged, except for some extra comments. The arguments struct is purely an internal detail of then_else_break_inner.

Most of this method's arguments are usually or always forwarded as-is to recursive invocations. Wrapping them in a dedicated struct allows us to document each struct field, and lets us use struct-update syntax to indicate which arguments are being modified when making a recursive call.

Nadrieril · 2024-03-04T14:45:32Z

Thank you!

@bors r+ rollup=always

bors · 2024-03-04T14:45:35Z

📌 Commit 4146136 has been approved by Nadrieril

It is now in the queue for this repository.

Extract an arguments struct for `Builder::then_else_break` Most of this method's arguments are usually or always forwarded as-is to recursive invocations. Wrapping them in a dedicated struct allows us to document each struct field, and lets us use struct-update syntax to indicate which arguments are being modified when making a recursive call. --- While trying to understand the lowering of `if` expressions, I found it difficult to keep track of the half-dozen arguments passed through to every call to `then_else_break`. I tried switching over to an arguments struct, and I found that it really helps to make sense of what each argument does, and how each call is modifying the arguments. I have some further ideas for how to streamline these recursive calls, but I've kept those out of this PR so that it's a pure refactoring with no behavioural changes.

…llaumeGomez Rollup of 9 pull requests Successful merges: - rust-lang#120976 (constify a couple thread_local statics) - rust-lang#121576 (Convert the rest of the visitors to use `VisitorResult`) - rust-lang#121826 (Use root obligation on E0277 for some cases) - rust-lang#121928 (Extract an arguments struct for `Builder::then_else_break`) - rust-lang#121958 (Fix redundant import errors for preload extern crate) - rust-lang#121959 (Removing absolute path in proc-macro) - rust-lang#121968 (Don't run test_get_os_named_thread on win7) - rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.) - rust-lang#121978 (Fix duplicated path in the "not found dylib" error) r? `@ghost` `@rustbot` modify labels: rollup

Extract an arguments struct for `Builder::then_else_break` Most of this method's arguments are usually or always forwarded as-is to recursive invocations. Wrapping them in a dedicated struct allows us to document each struct field, and lets us use struct-update syntax to indicate which arguments are being modified when making a recursive call. --- While trying to understand the lowering of `if` expressions, I found it difficult to keep track of the half-dozen arguments passed through to every call to `then_else_break`. I tried switching over to an arguments struct, and I found that it really helps to make sense of what each argument does, and how each call is modifying the arguments. I have some further ideas for how to streamline these recursive calls, but I've kept those out of this PR so that it's a pure refactoring with no behavioural changes.

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#120976 (constify a couple thread_local statics) - rust-lang#121683 (Fix LVI tests after frame pointers are enabled by default) - rust-lang#121703 (Add a way to add constructors for `rustc_type_ir` types) - rust-lang#121732 (Improve assert_matches! documentation) - rust-lang#121928 (Extract an arguments struct for `Builder::then_else_break`) - rust-lang#121939 (Small enhancement to description of From trait) - rust-lang#121968 (Don't run test_get_os_named_thread on win7) - rust-lang#121969 (`ParseSess` cleanups) - rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.) - rust-lang#121994 (Update platform-support.md with supported musl version) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#121928 - Zalathar:then-else-args, r=Nadrieril Extract an arguments struct for `Builder::then_else_break` Most of this method's arguments are usually or always forwarded as-is to recursive invocations. Wrapping them in a dedicated struct allows us to document each struct field, and lets us use struct-update syntax to indicate which arguments are being modified when making a recursive call. --- While trying to understand the lowering of `if` expressions, I found it difficult to keep track of the half-dozen arguments passed through to every call to `then_else_break`. I tried switching over to an arguments struct, and I found that it really helps to make sense of what each argument does, and how each call is modifying the arguments. I have some further ideas for how to streamline these recursive calls, but I've kept those out of this PR so that it's a pure refactoring with no behavioural changes.

rustbot assigned lcnr Mar 3, 2024

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 Mar 3, 2024

Nadrieril reviewed Mar 3, 2024
View reviewed changes

Comment threadcompiler/rustc_mir_build/src/build/matches/mod.rs Outdated

rustbot assigned Nadrieril and unassigned lcnr Mar 3, 2024

Zalathar force-pushed the then-else-args branch from 17b4bf3 to 0f1711dCompare March 4, 2024 02:25

Zalathar commented Mar 4, 2024
View reviewed changes

Comment threadcompiler/rustc_mir_build/src/build/expr/into.rs

Zalathar force-pushed the then-else-args branch from 0f1711d to 46cea0aCompare March 4, 2024 07:36

Zalathar force-pushed the then-else-args branch from 46cea0a to 4146136Compare March 4, 2024 07:42

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 Mar 4, 2024

GuillaumeGomez mentioned this pull request Mar 4, 2024
Rollup of 9 pull requests #121982
Closed

This was referenced Mar 4, 2024
Rollup of 10 pull requests #121995
Closed
Rollup of 10 pull requests #121998
Merged

bors merged commit 58a0f64 into rust-lang:masterMar 5, 2024

rustbot added this to the 1.78.0 milestone Mar 5, 2024

Zalathar deleted the then-else-args branch March 5, 2024 00:59

Navigation Menu

Uh oh!

Uh oh!

Extract an arguments struct for `Builder::then_else_break` - #121928

Extract an arguments struct for `Builder::then_else_break`#121928
bors merged 1 commit into
rust-lang:masterfrom
Zalathar:then-else-args

Zalathar commented Mar 3, 2024

Uh oh!

rustbot commented Mar 3, 2024

Uh oh!

rustbot commented Mar 3, 2024

Uh oh!

Zalathar commented Mar 3, 2024

Uh oh!

Uh oh!

Nadrieril commented Mar 3, 2024

Uh oh!

Zalathar commented Mar 4, 2024

Uh oh!

Uh oh!

Nadrieril commented Mar 4, 2024

Uh oh!

bors commented Mar 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Uh oh!

Conversation

Zalathar commented Mar 3, 2024

Uh oh!

rustbot commented Mar 3, 2024

Uh oh!

rustbot commented Mar 3, 2024

Uh oh!

Zalathar commented Mar 3, 2024

Uh oh!

Uh oh!

Nadrieril commented Mar 3, 2024

Uh oh!

Zalathar commented Mar 4, 2024

Uh oh!

Uh oh!

Nadrieril commented Mar 4, 2024

Uh oh!

bors commented Mar 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants