Skip to content

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