harm: Implement interesting methods for SBitValue - #45
Conversation
WalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
harm/src/bits.rs (2)
177-202: DRY: Delegate tofrom_shifted()instead of duplicating logic.Lines 189-201 duplicate the overflow checking logic from
from_shifted(). After validating alignment and converting to i32, you can simply callSelf::from_shifted(shifted_value).pub const fn new_i64(value: i64) -> Result<Self, BitError> { let shifted_value64 = value >> ALIGN; if value != shifted_value64 << ALIGN { return Err(BitError::Alignment { align: ALIGN as _ }); } // It would be nice to have a try_into here, but it's not const yet. // https://github.com/rust-lang/rust/issues/143773 let shifted_value: i32 = if (shifted_value64 as i32 as i64) == shifted_value64 { shifted_value64 as i32 } else { return Err(BitError::Unsupported); }; - // TODO comparing ranges may be cleaner, and produces exactly same code.- let upper_bits = i32::BITS - SIGNIFICANT_BITS;- let there_and_back_again = (shifted_value << upper_bits) >> upper_bits;- if shifted_value == there_and_back_again {- let mask = u32::MAX >> upper_bits;- let nested = (shifted_value as u32) & mask;- Ok(Self(nested))- } else {- Err(BitError::Overflow {- significant_bits: SIGNIFICANT_BITS as _,- align: ALIGN as _,- })- }+ Self::from_shifted(shifted_value) }
376-419: Consider adding a test for theUnsupportederror case.The tests cover valid i64 inputs and overflow scenarios, but there's no test exercising the
BitError::Unsupportedpath when the shifted i64 value exceeds i32 bounds.#[test]fntest_sbitvalue_new_i64_unsupported(){// Shifted value exceeds i32::MAXlet large:i64 = (i32::MAXasi64 + 1) << 2;let res = SBitValue::<30,2>::new_i64(large);assert_eq!(res,Err(BitError::Unsupported));}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/bits.rs(4 hunks)
🔇 Additional comments (2)
harm/src/bits.rs (2)
11-11: New error variant looks good.The
Unsupportedvariant appropriately captures the case where an i64 value's shifted representation exceeds i32 bounds. The display message is generic but sufficient.Also applies to: 31-33
158-175: Clean extraction of overflow validation logic.Refactoring to use
from_shifted()improves code organization and enables direct construction from pre-shifted values as described in the PR objectives.
cab1e4b to
6e7e402CompareThere was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
harm/src/bits.rs (2)
11-11: LGTM - Consider a more descriptive error message.The
Unsupportedvariant works correctly. The message "Unsupported value" is quite generic; you could consider something like "Shifted value exceeds i32 range" to help users diagnose issues more quickly, though the current message is acceptable.Also applies to: 31-33
409-416: Good test coverage - note on type configuration.This test correctly verifies that
new_i64returnsUnsupportedwhen the shifted value exceedsi32range.Note:
SBitValue<40, 0>would have issues iffrom_shiftedwere actually reached (the subtraction32 - 40inupper_bitswould underflow). The test works because the error is returned before that code path, as the comment suggests. This is fine for testing theUnsupportedpath, but consider using a valid type configuration likeSBitValue<31, 0>with a value like1i64 << 31to avoid any confusion.#[test] fn test_sbitvalue_new_i64_unsupported() { - // Actually, harm doesn't use wide types so wide.- type WideType = SBitValue<40, 0>;- let large_value: i64 = 1 << 31; // Does not fit into `i32`, but would fit the `WideType`.+ // Test that values exceeding i32 range return Unsupported+ type S31 = SBitValue<31, 0>;+ let large_value: i64 = 1 << 31; // Does not fit into `i32`- let res = WideType::new_i64(large_value);+ let res = S31::new_i64(large_value); assert_eq!(res, Err(BitError::Unsupported)); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/bits.rs(5 hunks)
🔇 Additional comments (4)
harm/src/bits.rs (4)
153-159: Good refactor to reduce duplication.Extracting the shifted-value validation into
from_shiftedand havingnewdelegate to it is a clean approach that avoids code duplication.
161-175: LGTM - Correct sign-extension validation.The round-trip check using arithmetic shifts correctly validates that the value fits within the signed range of
SIGNIFICANT_BITS. The masking logic properly extracts the significant bits.
177-190: LGTM - Correct i64-to-i32 range validation.The round-trip cast
(shifted_value64 as i32 as i64) == shifted_value64is the correct idiom for checking lossless i64→i32 conversion in a const context. The comment explaining the lack oftry_intodue to const limitations is helpful.
364-407: Comprehensive test coverage for the new constructors.The tests effectively cover:
- Basic equivalence with
new()for small values- Large positive values near the maximum
- Negative value representation (-1 producing all-ones)
- Boundary conditions for
from_shiftedWell done on the thorough testing.
+ `from_i64`: sometimes unshifted value fits only `i64` (for example, `AdrpOffset` implemented as an aligned `SBitValue<19, 12>`). + `from_shifted`: another way to construct such values.
6e7e402 to
883261fCompareThere was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
harm/src/bits.rs (1)
31-33: Consider a more descriptive error message.The error message "Unsupported value" is quite generic. Since
Unsupportedis specifically used when the shifted value doesn't fit ini32(as seen innew_i64), consider a more informative message like "Value exceeds i32 range after alignment shift" or similar.BitError::Unsupported => { - write!(f, "Unsupported value")+ write!(f, "Value exceeds i32 range after alignment shift") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/bits.rs(5 hunks)
🔇 Additional comments (4)
harm/src/bits.rs (4)
158-159: LGTM! Clean refactoring.The extraction of overflow checking logic into
from_shiftedimproves code modularity and enables constructingSBitValuefrom pre-shifted values.
161-175: LGTM! Correct sign-extension check.The method correctly validates that the shifted value fits within
SIGNIFICANT_BITSusing sign-extension. The mask logic properly isolates the lower bits for storage.
177-190: LGTM! Correct i64 handling with const-compatible conversion check.The method properly validates alignment, uses a clever round-trip cast to check if the i64 value fits in i32 (since
try_intoisn't const-stable yet), and delegates overflow checking tofrom_shifted. The logic correctly handles the case where the total bit range (SIGNIFICANT_BITS + ALIGN) may exceed 32 bits while keeping the shifted value within i32 bounds.
364-408: Comprehensive test coverage.The test suite thoroughly validates the new methods including basic functionality, alignment handling, large values, negative values, overflow conditions, and the unsupported case. The test assertions correctly verify the expected behavior and error conditions.
| #[test] | ||
| fn test_sbitvalue_new_i64_unsupported() { | ||
| // Actually, harm doesn't use wide types so wide. | ||
| type WideType = SBitValue<31, 0>; | ||
| let large_value: i64 = 1 << 31; // Does not fit into `i32`, but would fit the `WideType`. | ||
| let res = WideType::new_i64(large_value); | ||
| assert_eq!(res, Err(BitError::Unsupported)); | ||
| } |
There was a problem hiding this comment.
Clarify test comment.
The comment on line 411 states "Does not fit into i32, but would fit the WideType", but this isn't accurate. SBitValue<31, 0> has a range of -(1 << 30) to (1 << 30) - 1 (i.e., -1,073,741,824 to 1,073,741,823). The value 1 << 31 (2,147,483,648) exceeds this range as well. The test correctly validates the Unsupported error, but the comment may confuse readers about why this error occurs.
Consider updating the comment to clarify:
- // Actually, harm doesn't use wide types so wide.+ // Test case where the shifted value doesn't fit in i32.
type WideType = SBitValue<31, 0>;
- let large_value: i64 = 1 << 31; // Does not fit into `i32`, but would fit the `WideType`.+ let large_value: i64 = 1 << 31; // Exceeds i32::MAX, triggering Unsupported error🤖 Prompt for AI Agents
In harm/src/bits.rs around lines 409 to 416, the test comment wrongly claims the
value "Does not fit into `i32`, but would fit the `WideType`"; update the
comment to state that SBitValue<31, 0> has signed range -(1<<30) to (1<<30)-1
and that 1 << 31 (2,147,483,648) exceeds that range, so the test correctly
expects Err(BitError::Unsupported); change the comment to this clearer
explanation and leave the test logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
harm/src/bits.rs (1)
409-416: Clarify the test comment.The comment on line 411 ("Actually, harm doesn't use wide types so wide.") is incomplete and unclear. This issue was already flagged in a previous review.
As suggested in the previous review, consider updating to:
- // Actually, harm doesn't use wide types so wide.- type WideType = SBitValue<31, 0>;- let large_value: i64 = 1 << 31; // Does not fit into `i32`.+ // Test case where the shifted value doesn't fit in i32.+ type WideType = SBitValue<31, 0>;+ let large_value: i64 = 1 << 31; // Exceeds i32::MAX, triggering Unsupported error
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/bits.rs(5 hunks)
🧰 Additional context used
🪛 GitHub Actions: Rust
harm/src/bits.rs
[error] 29-33: cargo fmt --check failed. Misformatted Rust code detected in bits.rs; the Shifted value formatting should be written as a multi-line write! invocation. Command: 'cargo fmt -- --check'.
🔇 Additional comments (5)
harm/src/bits.rs (5)
11-11: LGTM!The new
Unsupportederror variant appropriately represents cases where the shifted value cannot fit in thei32implementation type.
153-159: LGTM!Good refactoring to delegate to
from_shiftedafter alignment validation. This reduces code duplication and improves maintainability.
161-175: LGTM!The sign-extension validation correctly handles both positive and negative shifted values, and the bit masking properly extracts the significant bits.
177-190: LGTM!The double-cast technique
(shifted_value64 as i32 as i64) == shifted_value64correctly validates whether the i64 value fits in i32 range, and the comment appropriately explains whytry_intocan't be used in const context.
364-407: LGTM!Comprehensive test coverage for both
new_i64andfrom_shiftedmethods, including valid cases, overflow scenarios, and edge cases with negative values and large positive values.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
harm/src/bits.rs (1)
412-419: Clarify theUnsupportedtest comment to describe why the error occursThe test logic for
test_sbitvalue_new_i64_unsupportedis correct, but the current comment ("Actually, harm doesn't use wide types so wide.") is a bit opaque and doesn’t explain that the failure is due to thei64→i32narrowing guard rather than the SBitValue type itself.Consider updating the comments to make the intent explicit, e.g.:
- // Actually, harm doesn't use wide types so wide.- type WideType = SBitValue<31, 0>;- let large_value: i64 = 1 << 31; // Does not fit into `i32`.+ // Test case where the shifted value is representable in the SBitValue type+ // but not in the intermediate i32 implementation type used by `new_i64`.+ type WideType = SBitValue<31, 0>;+ let large_value: i64 = 1 << 31; // Exceeds i32::MAX, triggering Unsupported error.This makes it clear that
BitError::Unsupportedis specifically about the implementation-type limitation, not about the generic SBitValue range itself.
🧹 Nitpick comments (2)
harm/src/bits.rs (2)
155-162:SBitValue::newdelegation andfrom_shiftedsemantics look correct; consider documenting the contractDelegating
newtofrom_shiftedpreserves the previous alignment check (valuemust round‑trip through>> ALIGN/<< ALIGN) and centralises the signed‑range + packing logic in one place, which is good.Since
from_shiftedispub constand does not re-check alignment, it might be worth adding a brief doc comment clarifying that:
shifted_valueis expected to be pre-shifted byALIGNbits (i.e. already aligned), and- this constructor only enforces that the value fits in
SIGNIFICANT_BITSas a signed integer, returningOverflowotherwise.That will help external callers avoid accidentally skipping the alignment invariant that
new/new_i64enforce.Also applies to: 164-178
180-193:new_i64narrowing logic is sound; optional doc + extra test could clarify intentThe
new_i64implementation mirrorsnew’s alignment check and the(shifted_value64 as i32 as i64) == shifted_value64pattern correctly guards the const narrowing fromi64toi32before delegating tofrom_shifted, which matches the intended “only if the shifted value fits in i32” contract.Two optional follow-ups you might consider:
- Add a doc comment explaining that
BitError::Unsupportedspecifically means “shifted value cannot be represented ini32for this implementation”, distinguishing it fromOverflowandSignOutRange.- Add a dedicated test where
valueis ani64that does fit intoi32but not intoSIGNIFICANT_BITS(sonew_i64returnsOverflow), to lock in that behaviour for thei64constructor as well.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/bits.rs(5 hunks)
🔇 Additional comments (3)
harm/src/bits.rs (3)
7-12:BitError::Unsupportedvariant and Display arm are consistent with new usageThe new
Unsupportedvariant cleanly separates “cannot be represented in implementation type” from overflow/alignment, and the correspondingDisplaymessage matches hownew_i64uses it, so this extension of the error surface looks coherent.Also applies to: 31-36
367-389:new_i64tests exercise the main success paths wellThe three
new_i64tests together cover:
- parity with
newfor a misaligned value,- a large positive value within the representable signed range after shifting, and
- a negative value exercising sign extension and masking.
That gives good confidence that
new_i64behaves consistently withnewandfrom_shiftedon the success paths.
391-410:from_shiftedvalid/invalid tests match the signed-range behaviourThe
from_shiftedtests correctly validate that:
SBitValue::<5, 2>::from_shifted(15)(just below the sign bit) encodes to raw bits0b01111, andSBitValue::<5, 2>::from_shifted(1 << 4)(value using the sign bit as magnitude) triggers anOverflowwith the expectedsignificant_bits/align.These align nicely with the sign-extension based range check in
from_shifted.
from_i64: sometimes unshifted value fits onlyi64(for example,AdrpOffsetimplemented as an alignedSBitValue<19, 12>).from_shifted: another way to construct such values.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.