feat: add checked_add method with overflow detection

[Topology2333]

PR Overview

Summary

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.

relates to #241

[peter-jerry-ye-code-review]

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.

[coveralls]

Pull Request Test Coverage Report for Build 440

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 88.352%

---

Totals
Change from base Build 438:	0.0%
Covered Lines:	3330
Relevant Lines:	3769

---

💛 - Coveralls

[peter-jerry-ye]

Hi, thank you for your contribution. But here seems to be a conterexample

test {
  let a = new_unchecked(922337203685477587L, 4)
  let b= new_unchecked(922337203685477587, 6)
  let c = a.checked_add(b)
  inspect(c, content="None") // should be 4611686018427387935 / 12
}

I'm closing this PR for now because we are going to move this package to x. You are welcome to contribute there.

[Topology2333]

Hi, thank you for your contribution. But here seems to be a conterexample

test {
  let a = new_unchecked(922337203685477587L, 4)
  let b= new_unchecked(922337203685477587, 6)
  let c = a.checked_add(b)
  inspect(c, content="None") // should be 4611686018427387935 / 12
}

I'm closing this PR for now because we are going to move this package to x. You are welcome to contribute there.

Thank you for your feedback!

You're absolutely right — the issue comes from the fact that I didn't reduce the operands before performing the intermediate multiplications. As a result, the unchecked multiplication overestimates the actual values, leading to a false overflow detection.

I'll work on improving the checked_add implementation by factoring out the GCD between the denominators before performing the multiplications, which should avoid the intermediate overflow in such cases.

Thanks again for pointing this out! Once the package is moved to x, I'll be happy to reopen a PR there with a corrected version.

[peter-jerry-ye]

I have migrated it to x : moonbitlang/x#161 and now it also supports int / int64 / bigint

I think you can checkout https://github.com/rust-num/num-rational for the correct implementation.