Newtype oparg align methods#7403
Conversation
📝 WalkthroughWalkthroughThis PR refactors the Label oparg type from a tuple struct with direct field access to a newtype with constructor and accessor methods. Multiple oparg types (Label, StoreFastLoadFast, ConstIdx, VarNum, LoadAttr, LoadSuperAttr) are reimplemented using a macro-driven newtype pattern. Call sites across codegen, compiler-core, jit, and vm are systematically updated to use Label::new() constructors and as_u32()/as_usize() accessors instead of tuple field access. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/vm/src/frame.rs (1)
6835-6846: Deduplicate the FOR_ITER exhaustion target adjustment.
for_iter_jump_on_exhausted()now re-implements the sameEND_FORskip logic asfor_iter_jump_target(). Reusing the helper would keep this control-flow rule in one place.♻️ Proposed cleanup
/// Handle iterator exhaustion in specialized FOR_ITER handlers. /// Skips END_FOR if present at target and jumps. fn for_iter_jump_on_exhausted(&mut self, target: bytecode::Label) { - let target_idx = target.as_usize(); - let jump_target = if let Some(unit) = self.code.instructions.get(target_idx) { - if matches!( - unit.op, - bytecode::Instruction::EndFor | bytecode::Instruction::InstrumentedEndFor - ) { - bytecode::Label::new(target.as_u32() + 1) - } else { - target - } - } else { - target - }; - self.jump(jump_target); + self.jump(self.for_iter_jump_target(target)); }Also applies to: 8817-8832
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 6835 - 6846, The FOR_ITER exhaustion target adjustment logic is duplicated between for_iter_jump_target and for_iter_jump_on_exhausted; refactor so the rule lives in one place by making for_iter_jump_on_exhausted call for_iter_jump_target (or extract a shared helper used by both). Locate the duplicate logic in functions for_iter_jump_target and for_iter_jump_on_exhausted and remove the repeated END_FOR/InstrumentedEndFor check from for_iter_jump_on_exhausted, delegating to for_iter_jump_target to perform the END_FOR skip and return the adjusted bytecode::Label.crates/compiler-core/src/bytecode/oparg.rs (1)
702-705:From<$name> for usizebakes in a pointer-width assumption.
stdintentionally does not provideFrom<u32> for usize; this macro reintroduces the same potentially truncating cast for every oparg wrapper. I'd keepas_usize()as the explicit conversion and only add a trait impl here if you want a checkedTryFrom.Suggested change
- impl From<$name> for usize { - fn from(value: $name) -> Self { - value.as_usize() - } - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 702 - 705, The impl From<$name> for usize bakes in a pointer-width assumption and can truncate on some platforms; remove that impl and keep callers using the explicit as_usize() method on the wrapper ($name.as_usize()). If you need a fallible/checked conversion, add a TryFrom<$name> for usize implementation instead (implement TryFrom::try_from for $name that performs a safe/checked conversion and returns a Result), but do not reintroduce the infallible From<$name> for usize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 702-705: The impl From<$name> for usize bakes in a pointer-width
assumption and can truncate on some platforms; remove that impl and keep callers
using the explicit as_usize() method on the wrapper ($name.as_usize()). If you
need a fallible/checked conversion, add a TryFrom<$name> for usize
implementation instead (implement TryFrom::try_from for $name that performs a
safe/checked conversion and returns a Result), but do not reintroduce the
infallible From<$name> for usize.
In `@crates/vm/src/frame.rs`:
- Around line 6835-6846: The FOR_ITER exhaustion target adjustment logic is
duplicated between for_iter_jump_target and for_iter_jump_on_exhausted; refactor
so the rule lives in one place by making for_iter_jump_on_exhausted call
for_iter_jump_target (or extract a shared helper used by both). Locate the
duplicate logic in functions for_iter_jump_target and for_iter_jump_on_exhausted
and remove the repeated END_FOR/InstrumentedEndFor check from
for_iter_jump_on_exhausted, delegating to for_iter_jump_target to perform the
END_FOR skip and return the adjusted bytecode::Label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e1a75ba9-493f-4eb0-99ba-b6eefb796523
📒 Files selected for processing (6)
crates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/oparg.rscrates/jit/src/instructions.rscrates/vm/src/frame.rs
Summary by CodeRabbit