Conversation
📝 WalkthroughWalkthroughThis PR refactors the bytecode instruction type system by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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/compiler-core/src/bytecode/oparg.rs (1)
757-771: Use named constants for nibble packing to remove magic numbers.Logic is correct, but replacing
4and15with constants makes the encoding contract clearer and safer to maintain.Suggested diff
impl VarNums { + const LOW_NIBBLE_MASK: u32 = 0x0F; + const HIGH_NIBBLE_SHIFT: u32 = 4; + #[must_use] pub const fn idx_1(self) -> VarNum { - VarNum::new(self.0 >> 4) + VarNum::new(self.0 >> Self::HIGH_NIBBLE_SHIFT) } #[must_use] pub const fn idx_2(self) -> VarNum { - VarNum::new(self.0 & 15) + VarNum::new(self.0 & Self::LOW_NIBBLE_MASK) } #[must_use] pub const fn indexes(self) -> (VarNum, VarNum) { (self.idx_1(), self.idx_2()) } }🤖 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 757 - 771, Replace the magic numbers in VarNums's nibble packing/unpacking by adding descriptive consts (e.g., NIBBLE_BITS = 4 and NIBBLE_MASK = (1 << NIBBLE_BITS) - 1) and use them in the methods idx_1, idx_2, and indexes; update VarNums::idx_1 to shift by NIBBLE_BITS and VarNums::idx_2 to mask with NIBBLE_MASK while keeping them as const fn and returning VarNum::new(...) so the encoding contract is explicit and easier to maintain.crates/compiler-core/src/bytecode/instruction.rs (1)
1348-1353: Inconsistent display format forStoreFastLoadFast.
StoreFastLoadFastoutputs raw index numbers (store_idx, load_idx) instead of variable names, while similar instructions likeLoadFastLoadFast,LoadFastBorrowLoadFastBorrow, andStoreFastStoreFastall usevarname()to display human-readable variable names.Consider using
varname()for consistency:♻️ Suggested fix for consistency
Self::StoreFastLoadFast { var_nums } => { let oparg = var_nums.get(arg); let (store_idx, load_idx) = oparg.indexes(); - write!(f, "STORE_FAST_LOAD_FAST")?; - write!(f, " ({}, {})", store_idx, load_idx) + write!( + f, + "{:pad$}({}, {})", + "STORE_FAST_LOAD_FAST", + varname(store_idx), + varname(load_idx) + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 1348 - 1353, The Display implementation for the enum variant Self::StoreFastLoadFast { var_nums } prints raw indexes (store_idx, load_idx) rather than human-readable names; change the second write! call in that match arm to use varname() on the indexes (the same approach used by LoadFastLoadFast, LoadFastBorrowLoadFastBorrow, and StoreFastStoreFast) so it prints the variable names (e.g., var_nums.varname(store_idx), var_nums.varname(load_idx)) instead of numeric indices while preserving the "STORE_FAST_LOAD_FAST" label.
🤖 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/instruction.rs`:
- Around line 1348-1353: The Display implementation for the enum variant
Self::StoreFastLoadFast { var_nums } prints raw indexes (store_idx, load_idx)
rather than human-readable names; change the second write! call in that match
arm to use varname() on the indexes (the same approach used by LoadFastLoadFast,
LoadFastBorrowLoadFastBorrow, and StoreFastStoreFast) so it prints the variable
names (e.g., var_nums.varname(store_idx), var_nums.varname(load_idx)) instead of
numeric indices while preserving the "STORE_FAST_LOAD_FAST" label.
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 757-771: Replace the magic numbers in VarNums's nibble
packing/unpacking by adding descriptive consts (e.g., NIBBLE_BITS = 4 and
NIBBLE_MASK = (1 << NIBBLE_BITS) - 1) and use them in the methods idx_1, idx_2,
and indexes; update VarNums::idx_1 to shift by NIBBLE_BITS and VarNums::idx_2 to
mask with NIBBLE_MASK while keeping them as const fn and returning
VarNum::new(...) so the encoding contract is explicit and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d09d8e27-02fc-4171-8d40-a1b168537389
📒 Files selected for processing (4)
crates/compiler-core/src/bytecode/instruction.rscrates/compiler-core/src/bytecode/oparg.rscrates/jit/src/instructions.rscrates/vm/src/frame.rs
| #[derive(Clone, Copy)] | ||
| #[repr(transparent)] | ||
| pub struct LoadAttr(u32) | ||
| pub struct VarNums(u32) |
There was a problem hiding this comment.
This is actually a pair of u16, right? can this be written using union?
There was a problem hiding this comment.
Maybe this can be written as a union, I'm not familiar enough with them to properly implement it. Also, I ain't sure how much this will help performance/memory utilization, in comparison with the effort to refactor things (if needed?)
Summary by CodeRabbit