-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Matching classes causes a stack underflow, no clue why. That's for a different pr though. |
There was a problem hiding this 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.
match n { | ||
// If no name is provided, simply pop the top of the stack. | ||
None => { | ||
emit!(self, Instruction::Pop); | ||
Ok(()) | ||
} |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
#5628 was a little chaotic.
Instruction::IsOperation
todo!
)extra_tests
and the test only checks for compile.