Skip to content

Cleanup match statement codegen #5700

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

Merged
merged 10 commits into from
Apr 18, 2025

Conversation

arihant2math
Copy link
Collaborator

@arihant2math arihant2math commented Apr 15, 2025

#5628 was a little chaotic.

  • Remove Instruction::IsOperation
  • Cleans up some error handling (less panicking due to todo!)
  • Handle stack underflows with more informative error
  • Makes code more rust-like
  • Cleans up variable names for spellcheck
  • Removes match test because we have a test in extra_tests and the test only checks for compile.

@arihant2math arihant2math marked this pull request as ready for review April 15, 2025 18:57
@arihant2math arihant2math changed the title Cleanup match statement Cleanup match statement codegen Apr 15, 2025
@arihant2math
Copy link
Collaborator Author

Matching classes causes a stack underflow, no clue why. That's for a different pr though.

Copy link
Member

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

lgtm! lmk when you feel it's ready to be merged.

Comment on lines +1947 to +1952
match n {
// If no name is provided, simply pop the top of the stack.
None => {
emit!(self, Instruction::Pop);
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

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

You could also use let-else, to prevent rightward drift.

        let Some(name) = n else {
            // If no name is provided, simply pop the top of the stack.
            emit!(self, Instruction::Pop);
            return Ok(());
        };

@@ -2155,10 +2158,7 @@ impl Compiler<'_> {
for ident in attrs.iter().take(n_attrs).skip(i + 1) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't the code you changed, but if you're looking to make it more rusty, you could use .enumerate() here for the outer loop (for (i, attr) in attrs.iter().enumerate()) and then just for ident in &attrs[i + 1..] for the inner loop.

@coolreader18 coolreader18 merged commit 0d4faa0 into RustPython:main Apr 18, 2025
11 checks passed
@arihant2math arihant2math deleted the match-cleanup branch April 19, 2025 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants