GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Reduce usage `JumpIfTrueOrPop` & `JumpIfFalseOrPop` opcdes. Refactor `compile_compare` by ShaharNaveh · Pull Request #6524 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughThis PR refactors comparison operation compilation in the bytecode generator, introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…p-if-true-false-pop
…p-if-true-false-pop
JumpIfTrueOrPop & JumpIfFalseOrPop opcdesJumpIfTrueOrPop & JumpIfFalseOrPop opcdes. Refactor compile_compare
|
This started as an attempt to remove |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
4118-4126: Commented-out code and inconsistent refactoring with PR goal.The PR objective is to reduce usage of
JumpIfFalseOrPopopcode, but:
- Lines 4118-4123 contain commented-out code showing the intended replacement pattern
- Line 4125 still uses
JumpIfFalseOrPopThis appears to be an intentional reversion based on commit history ("Reverted PopJumpIfFalse changes for comparisons"), but the commented code creates noise and the inconsistency should be documented. Consider either:
- Removing the commented code if the reversion is permanent
- Adding a TODO/FIXME comment explaining why
JumpIfFalseOrPopis still used here🔎 Clean up commented code
self.compile_addcompare(op); // if comparison result is false, we break with this value; if true, try the next one. - /* - emit!(self, Instruction::CopyItem { index: 1 }); - // emit!(self, Instruction::ToBool); // TODO: Uncomment this - emit!(self, Instruction::PopJumpIfFalse { target: cleanup }); - emit!(self, Instruction::PopTop); - */ - + // NOTE: Using JumpIfFalseOrPop here instead of CopyItem+PopJumpIfFalse+PopTop + // because chained comparisons require the comparison result to remain on stack + // for cleanup when short-circuiting. emit!(self, Instruction::JumpIfFalseOrPop { target: cleanup });
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/codegen/src/compile.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/codegen/src/compile.rs
🔇 Additional comments (4)
crates/codegen/src/compile.rs (4)
4049-4066: Clean helper extraction for comparison operations.The
compile_addcomparemethod is a well-structured helper that mapsCmpOpvariants to their corresponding bytecode instructions. This follows the CPython reference and improves code organization.
4467-4491: Correct implementation of the opcode replacement pattern.The refactoring correctly replaces
JumpIfFalseOrPop/JumpIfTrueOrPopwith the explicit sequence:
CopyItem { index: 1 }- duplicates TOSPopJumpIfFalse/PopJumpIfTrue- conditional jump, pops TOSPopTop- cleans up if no jump occurredThis properly implements short-circuit evaluation:
- For
and: if any value is false, jump with that value on stack- For
or: if any value is true, jump with that value on stackThe semantics are preserved while using more explicit opcodes.
4086-4141: The refactoredcompile_comparecorrectly aligns with CPython's chained comparison behavior.The stack manipulation pattern with
Swap { index: 2 }andCopyItem { index: 2 }achieves the same effect as CPython'sDUP_TOPandROT_THREEinstructions. Both approaches preserve the middle operand for the next comparison in the chain. The cleanup block correctly handles early exit by restoring stack invariants when a comparison result is false. The implementation usingJumpIfFalseOrPopmatches CPython's semantics for conditional chaining.
4033-4035: Verify stack cleanup when guard evaluates to false in default case.When the default case guard evaluates to false and jumps to
end(line 4039), the guard result is left on the stack:
- After
CopyItem:[guard_result, guard_result]- After
PopJumpIfFalse(false branch): pops the copy, leaving[guard_result]PopToponly executes on true pathThe
endblock performs no stack cleanup, so verify whether the guard result should be popped in the false path or if this is intentional handling.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| } | ||
| ); | ||
| } | ||
| /* |
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.
what was the blocker of this?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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.
That's the only reason why both opcodes stayed:/
This change caused regression for
RustPython/Lib/test/test_compile.py
Lines 1073 to 1081 in 61ddd98
| def test_no_wraparound_jump(self): | |
| # See https://bugs.python.org/issue46724 | |
| def while_not_chained(a, b, c): | |
| while not (a < b < c): | |
| pass | |
| for instr in dis.Bytecode(while_not_chained): | |
| self.assertNotEqual(instr.opname, "EXTENDED_ARG") |
Not because it found an EXTENDED_ARG, but because iterating over dis.Bytecode(...) gave an error
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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.
if that's because our test_compile is an old one, replacing it will be also good.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👀 1 reaction
| } | ||
| ); | ||
| } | ||
| /* |
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.
if that's because our test_compile is an old one, replacing it will be also good.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👀 1 reaction
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.