8000
feat: add checked_add method with overflow detection by Topology2333 · Pull Request #2284 · moonbitlang/core · GitHub
More Web Proxy on the site http://driver.im/
You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
Add checked_add to Rational for safe addition with Int64 overflow detection. Returns None if intermediate operations overflow, otherwise returns a reduced rational.
Changes
Add T::checked_add(self, other) -> T?
Handle overflow in numerator and denominator computation
Add unit tests for normal and overflow cases
Motivation
op_add does not check for overflow. checked_add offers a safe alternative for critical use cases.
Potential missing case in overflow detection for numerator addition
Category
Correctness Code Snippet
let num = lhs + rhs
if (lhs ^ rhs) >= 0L && (lhs ^ num) < 0L {
return None
} Recommendation
Consider adding explicit checks for lhs/rhs being INT64_MIN to handle edge cases Reasoning
The current overflow check uses sign bit XOR which works for most cases, but could potentially miss edge cases when one operand is INT64_MIN. Explicit checks would make the overflow detection more robust.
Variable naming could be more descriptive
Category
Maintainability Code Snippet
let (a, b) = (self, other)
let lhs = a.numerator * b.denominator Recommendation
Use more descriptive names like first and second or keep original self/other names Reasoning
Single letter variables like 'a' and 'b' reduce code readability. More descriptive names would make the code's intent clearer, especially for complex rational number operations.
Test coverage could be more comprehensive
Category
Maintainability Code Snippet
test "checked_add" {
// Only tests 3 cases
} Recommendation
Add more test cases covering edge cases like:
Adding with zero
INT64_MIN denominators
Denominator overflow cases Reasoning
Current tests cover basic functionality and one overflow case. More comprehensive testing of edge cases would help ensure the implementation is robust across all scenarios.
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.
PR Overview
Summary
Add
checked_addtoRationalfor safe addition withInt64overflow detection. ReturnsNoneif intermediate operations overflow, otherwise returns a reduced rational.Changes
T::checked_add(self, other) -> T?Motivation
op_adddoes not check for overflow.checked_addoffers a safe alternative for critical use cases.relates to #241