fix(shifter): use saturating arithmetic for sequential shifts#513
fix(shifter): use saturating arithmetic for sequential shifts#513EffortlessSteven wants to merge 1 commit into
Conversation
`ShifterInput::from_sequential` previously computed `current_gear + 1` and `current_gear - 1` before clamping. At `i32::MAX` (upshift) or `i32::MIN` (downshift) those raw arithmetic operations panic in debug builds via the integer-overflow check; in release builds they wrap. Both are wrong for a sequential shifter that should saturate to the gear-range clamp. Switches to `saturating_add` / `saturating_sub` so the clamp can do its job at the i32 extremes too. Adds 16 coverage tests in tests/coverage_gaps.rs that pin: - Saturating up/downshift at `i32::MAX` / `i32::MIN` (regression guard). - Up/downshift boundaries just inside the gear-range clamp (current=7 upshift → 8; current=2 downshift → 1). - No-shift passthrough preserves `i32::MAX` / `i32::MIN`. - `GearPosition::new(i32::MAX)` is forward, `i32::MIN` is reverse. - `parse_gamepad` ignores header bytes 0 and 1. - `parse_gamepad` paddle-button-mask isolation: `0x10` and `0x20` alone control the paddle flags; all other bits are ignored. - `ShifterError` implements `std::error::Error` (source is None for all variants), and the `InvalidGear` payload is recoverable via pattern match for `i32::MIN`, `-1`, `0`, `100`, `i32::MAX`. - `ShifterCapabilities::default()` field-by-field assertion. Verified with `cargo test -p openracing-shifter` (190 tests pass) and `cargo clippy -p openracing-shifter --tests -- -D warnings`.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
Compatibility Layer Usage ReportCurrent usage count: 193 📈 Usage Trend (Last 30 Days)
Usage Details
... and 183 more occurrences Migration GuideTo migrate these usages, replace:
See Migration Patterns for detailed examples. |
2 participants
Summary
While auditing
openracing-shifterfor coverage gaps I found a real overflow bug inShifterInput::from_sequentialthat the existing suite did not exercise:At
i32::MAX(upshift) ori32::MIN(downshift), the raw arithmetic happens before the clamp. In debug builds Rust panics on the integer overflow; in release builds it wraps. Both are wrong for a saturating sequential shifter.The fix is
saturating_add/saturating_sub, so the gear-range clamp can still do its job at the extremes.The PR also adds 16 coverage tests covering this regression plus several other gaps the audit surfaced.
What changed
crates/openracing-shifter/src/input.rs—(current_gear + 1)/(current_gear - 1)→saturating_add(1)/saturating_sub(1).crates/openracing-shifter/tests/coverage_gaps.rs(new) — 16 tests covering:i32::MAXsaturates toMAX_GEARS; downshift ati32::MINsaturates to1. Without the fix this panics in debug.7upshift →8; current2downshift →1. The original suite tested at-or-above the clamp, never just below.i32::MAX/i32::MIN(no clamp applied when neither paddle pressed).GearPosition::newati32::MAX(forward only) andi32::MIN(reverse).parse_gamepadheader-byte isolation — bytes 0 and 1 do not affect output.parse_gamepadbutton-mask isolation — bits other than0x10(paddle up) and0x20(paddle down) are ignored, and the paddle bits are correctly isolated when neighbouring bits are also set.ShifterErrorstd::error::Errorimpl — coerces to&dyn Error,source()returnsNonefor all variants,InvalidGear(i32)payload is recoverable via pattern match fori32::MIN,-1,0,100,i32::MAX.ShifterCapabilities::default()— every field asserted explicitly.Verification
Test plan
cargo test -p openracing-shiftercargo clippy -p openracing-shifter --tests -- -D warningsfrom_sequential_upshift_at_i32_max_saturates_to_max_gearsfails onmain(verifies regression guard) — pre-fix it would panic in debug or wrap in releasehttps://claude.ai/code/session_01NZ5jzdE2H3bbuPuYqbjCnh
Generated by Claude Code