Skip to content

Avoid terminator: None in custom mir parser - #165502

Open
panstromek wants to merge 2 commits into
rust-lang:mainfrom
panstromek:new-terminator-option-1
Open

panstromek wants to merge 2 commits into
rust-lang:mainfrom
panstromek:new-terminator-option-1

Conversation

@panstromek

@panstromek panstromek commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

First step towards replacing Option<Terminator> with Terminator in BasicBlockData. See this thread for more info.

This is a slight restructuring of the custom mir parser that avoids creating dummy basic blocks that don't have terminators. More details in individual commits.

@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 Nov 5, 2026
@rustbot

rustbot commented Nov 5, 2026

Copy link
Copy Markdown
Collaborator

r? @nnethercote

rustbot has assigned @nnethercote.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler, mir
  • compiler, mir expanded to 77 candidates
  • Random selection from 20 candidates

@panstromek panstromek changed the title Avoid terminator: None in custom mir parsing Avoid terminator: None in custom mir parser Nov 5, 2026
@rust-log-analyzer

This comment has been minimized.

…minator

This was only needed because `is_cleanup` property was parsed separately from the basic block data, so we do this by parsing `is_cleanup` into a vec of bools first, and later transform it into the bb data when parsing blocks themselves.
@panstromek
panstromek force-pushed the new-terminator-option-1 branch from 16f2ca6 to ca5705a Compare November 5, 2026 15:02

@hanna-kruppe hanna-kruppe left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks correct, but I have two questions.

r? me @rustbot author

View changes since this review

Comment on lines +187 to +189
// just a sanity check to make sure order of bbs
// in cleanup blocks matches the one in the body
assert_eq!(inserted_bb, bb);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what this checks / what "order of BBs in cleanup blocks" means. We're comparing bb = i, which we get from enumerate of block_defs, with the index in the body's basic block list. That should always be true, unless something sneakily pushes another basic block before this loop or inside parse_block_def. Maybe that's what you had in mind, but either way the comment needs to be clearer.

thir: &'a Thir<'tcx>,
source_scope: SourceScope,
body: &'a mut Body<'tcx>,
cleanup_blocks: IndexVec<BasicBlock, bool>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be a GrowableBitSet? The memory footprint doesn't matter, but it seems conceptually more appropriate than IndexVec<_, bool>.

@rustbot rustbot assigned hanna-kruppe and unassigned nnethercote Nov 7, 2026
@rustbot rustbot 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 Nov 7, 2026
@rustbot

rustbot commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rust-bors

rust-bors Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #165597) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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