harm: refactor tbz/tbnz - #48
Conversation
+ Use type aliases. + Generalize on offsets. + Make tests more readable.
📝 WalkthroughWalkthroughIntroduces a generic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (10)
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harm/src/instructions/control/testbranch.rs (1)
49-60: Align 32-bit trait bounds with 64-bit implementation.The 32-bit
MakeTestBranchimplementation usesInto<RegOrZero32>and.into(), while the 64-bit version usesIntoReg<RegOrZero64>and.into_reg(). SinceIntoRegis implemented forRegOrZero32, update the 32-bit implementation to useIntoReg<RegOrZero32>and call.into_reg()for consistency.
🧹 Nitpick comments (2)
harm/src/instructions/control/testbranch.rs (2)
36-47: Inconsistent type alias usage.Lines 37 and 39 use the raw
UBitValue<6>type instead of theTestBit64alias, which is inconsistent with the alias definition and its usage on line 36.🔎 Suggested fix
impl<R: IntoReg<RegOrZero64>> MakeTestBranch<R, TestBit64, TestOffset> - for TestBranch<RegOrZero64, UBitValue<6>, TestOffset>+ for TestBranch<RegOrZero64, TestBit64, TestOffset> { - fn new(op: bool, reg: R, bit: UBitValue<6>, offset: TestOffset) -> Self {+ fn new(op: bool, reg: R, bit: TestBit64, offset: TestOffset) -> Self { Self { op, reg: reg.into_reg(), bit, offset, } } }
110-121: Consider addingtbnztests.All current tests cover
tbzonly. Whiletbnzshares the encoding logic (differing only in theopbit), adding at least onetbnztest would improve coverage and serve as a regression guard.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/instructions/control/testbranch.rs
🔇 Additional comments (5)
harm/src/instructions/control/testbranch.rs (5)
18-21: LGTM!The type aliases improve readability and provide meaningful names for the bit-width constraints.
22-30: LGTM!The struct generalization and sealed implementations correctly constrain the valid instantiations.
62-90: LGTM!The bit extraction logic correctly handles the encoding differences between 64-bit (6-bit index, b5 can be 0 or 1) and 32-bit (5-bit index, b5 always 0) variants.
92-108: LGTM!The generalized constructors properly leverage the trait bounds for type-safe instruction construction.
123-177: LGTM!Tests provide good coverage for:
- 64-bit registers with both high (≥32) and low (<32) bit positions
- 32-bit registers
- Zero registers (XZR, WZR)
The compact
inst!()macro format improves readability as noted in PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harm/src/instructions/control/testbranch.rs (1)
36-60: Fix trait bound inconsistency: useIntoReg<RegOrZero32>instead ofInto<RegOrZero32>for the 32-bit implementation.Line 49 uses the standard
Into<RegOrZero32>trait while line 36 uses the customIntoReg<RegOrZero64>trait. SinceIntoReg<RegOrZero32>has proper implementations forReg32andRegOrZero32and is used consistently throughout the codebase for register conversions, the 32-bit variant should also useIntoReg<RegOrZero32>withinto_reg()(line 55) to match the 64-bit pattern.
🧹 Nitpick comments (1)
harm/src/instructions/control/testbranch.rs (1)
123-177: LGTM: Tests updated correctly with improved readability.The tests now use explicit type aliases, making them more self-documenting. Coverage includes both 64-bit and 32-bit variants, different bit positions, and zero registers.
Optional: Consider adding test coverage for negative offsets.
All tests currently use positive offset 76. Since
TestBranchOffsetis a signed value (SBitValue<14, 2>), adding at least one test with a negative offset would improve coverage.Example test for negative offset
#[test]fntest_tbz_64_neg_offset(){let offset = TestBranchOffset::new(-8).unwrap();let bit = TestBranchBit64::new(10).unwrap();let it = tbz(X5, bit, offset);let words:Vec<_> = it.encode().collect();// Verify expected encoding for negative offsetassert_eq!(words, inst!(0x...));// Fill in expected value}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/instructions/control/testbranch.rs
🧰 Additional context used
🧬 Code graph analysis (1)
harm/src/instructions/control/testbranch.rs (3)
harm/src/instructions/control/branch_reg.rs (2)
new(26-28)reg(31-34)harm/src/instructions/control/branch_imm.rs (5)
new(200-200)new(209-215)new(226-232)new(241-247)new(256-262)harm/src/instructions/arith/add.rs (3)
new(42-42)new(57-59)new(66-68)
🔇 Additional comments (7)
harm/src/instructions/control/testbranch.rs (7)
18-20: LGTM: Type aliases improve clarity.The type aliases appropriately encode ARM64 test-and-branch constraints (6-bit for 64-bit registers, 5-bit for 32-bit registers, 14-bit signed offset with 4-byte alignment).
22-27: LGTM: Generalization improves flexibility.Adding the
Offsettype parameter allows for future extensibility while maintaining type safety through trait bounds.
29-30: LGTM: Sealed trait pattern correctly applied.The explicit implementations restrict
MakeTestBranchto only the two intended concrete types.
32-34: LGTM: Trait signature correctly updated.The trait now accepts the generic
Offsetparameter, consistent with the struct definition.
62-75: LGTM: Bit extraction logic is correct.The 6-bit value is correctly split into b5 (MSB) and b40 (bits 4-0) for ARM64 test-and-branch encoding.
77-90: LGTM: 32-bit encoding correctly sets b5 to 0.The implementation correctly handles 5-bit test positions (0-31) by hardcoding b5=0, since bit 5 doesn't exist for 32-bit registers.
92-108: LGTM: Public API correctly generalized.The functions now accept a generic
Offsetparameter with appropriate trait bounds, improving flexibility while maintaining type safety.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.