fix: correct int_signum! handling of minimum signed integer values#24
Merged
fix: correct int_signum! handling of minimum signed integer values#24
Conversation
The int_signum! macro incorrectly computed the absolute value of minimum signed integers (e.g., i8::MIN = -128). Using wrapping_neg() on these values returns the same negative value due to overflow, and casting to u64 then sign-extends incorrectly. Fix: Use unsigned_abs() which correctly handles all values including the minimum of each signed type. Also adds QuickCheck test for i8 orient2d and increases all QuickCheck tests to run 10,000 iterations for more thorough coverage.
Add BigInt reference implementations and QuickCheck property tests for i8::cmp_dist and i8::incircle predicates. Also fix overflow bug in i8::incircle where subtractions were performed outside the int_signum! macro, causing overflow for extreme i8 values.
Move all coordinate subtractions inside the apfp_signum!/int_signum! macros across all geometry modules (f64, i16, i32, i64). This ensures the subtractions are computed with arbitrary precision arithmetic, avoiding potential overflow (for integers) or precision loss (for f64). The i8 module was already fixed in the previous commit. Also adds incircle benchmark for f64 comparing apfp, robust crate, naive f64, and rational implementations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
i8::MIN = -128) in theint_signum!macroorient2dthat detected this bugBug Details
The
int_signum!macro usedwrapping_neg()to compute absolute values when converting signed integers toBigInt. For minimum values like-128i8,wrapping_neg()returns-128(since128cannot be represented ini8), and thenas u64sign-extends this to an incorrect large value.Fix: Replace
wrapping_neg() as u64withunsigned_abs() as u64, which correctly handles all values including the minimum of each signed type.