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

match lowering: Split off test_candidates into several functions and improve comments #121374

Merged
merged 2 commits into from
Feb 25, 2024

Conversation

Nadrieril
Copy link
Member

The logic of test_candidates has three steps: pick a test, sort the candidates, and generate code for everything. So I split it off into three methods.

I also ended up reworking the comments that explain the algorithm. In particular I added detailed examples. I removed the digression about #29740 because it's no longer relevant to how the code is structured today.

r? @matthewjasper

@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 Feb 21, 2024
@rust-log-analyzer

This comment was marked as resolved.

@Nadrieril Nadrieril changed the title Split off test_candidates into several functions and improve comments match lowering: Split off test_candidates into several functions and improve comments Feb 21, 2024
Copy link
Contributor

@matthewjasper matthewjasper 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 with or without these addressed

compiler/rustc_mir_build/src/build/matches/mod.rs Outdated Show resolved Hide resolved
// of the main candidate; but for a test like SwitchInt, we
// may want to add cases based on the candidates that are
debug!("test_candidates: test={:?} match_pair={:?}", test, match_pair);
// Most of the time, the test to perform is simply a function of the main candidate; but for
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment doesn't seem to be in the correct place and can probably just be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feels alright to me, it's trying to explain the need for add_cases_to_X shenanigans

Co-authored-by: matthewjasper <20113453+matthewjasper@users.noreply.github.com>
@Nadrieril
Copy link
Member Author

Thank you!

@bors r=@matthewjasper

@bors
Copy link
Contributor

bors commented Feb 24, 2024

📌 Commit de4efa5 has been approved by matthewjasper

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 24, 2024
…wjasper

match lowering: Split off `test_candidates` into several functions and improve comments

The logic of `test_candidates` has three steps: pick a test, sort the candidates, and generate code for everything. So I split it off into three methods.

I also ended up reworking the comments that explain the algorithm. In particular I added detailed examples. I removed the digression about rust-lang#29740 because it's no longer relevant to how the code is structured today.

r? `@matthewjasper`
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 24, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121343 (Add examples for some methods on slices)
 - rust-lang#121374 (match lowering: Split off `test_candidates` into several functions and improve comments)
 - rust-lang#121474 (Ignore compiletest test directive migration commits)
 - rust-lang#121515 (promotion: don't promote int::MIN / -1)
 - rust-lang#121530 (Fix incorrect doc of ScopedJoinHandle::is_finished)
 - rust-lang#121551 (Forbid use of `extern "C-unwind"` inside standard library)
 - rust-lang#121556 (Use `addr_of!`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f4ba47f into rust-lang:master Feb 25, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 25, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 25, 2024
Rollup merge of rust-lang#121374 - Nadrieril:factor-explain, r=matthewjasper

match lowering: Split off `test_candidates` into several functions and improve comments

The logic of `test_candidates` has three steps: pick a test, sort the candidates, and generate code for everything. So I split it off into three methods.

I also ended up reworking the comments that explain the algorithm. In particular I added detailed examples. I removed the digression about rust-lang#29740 because it's no longer relevant to how the code is structured today.

r? ``@matthewjasper``
@Nadrieril Nadrieril deleted the factor-explain branch February 25, 2024 09:33
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.

5 participants