8000 feat(bigint): add `BigInt::to_int16` and `BitInt::to_uint16` by illusory0x0 · Pull Request #2387 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(bigint): add BigInt::to_int16 and BitInt::to_uint16 #2387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 2, 2025

Conversation

illusory0x0
Copy link
Contributor

No description provided.

Copy link
Potential overflow behavior not properly handled for large BigInt values

Category
Correctness
Code Sni 8000 ppet
pub fn BigInt::to_uint16(self : BigInt) -> UInt16 {
self.to_int().to_uint16()
}

pub fn BigInt::to_int16(self : BigInt) -> Int16 {
self.to_int().to_int16()
}
Recommendation
Consider implementing direct conversion that handles BigInt values larger than Int range, or add explicit documentation about the intermediate Int conversion limitation
Reasoning
The current implementation first converts BigInt to Int, which may overflow for very large BigInt values before reaching the 16-bit conversion. This could lead to unexpected results when the BigInt is larger than Int.max_value but the caller only wants the lower 16 bits.

Code duplication in conversion pattern

Category
Maintainability
Code Snippet
pub fn BigInt::to_uint16(self : BigInt) -> UInt16 {
self.to_int().to_uint16()
}

pub fn BigInt::to_int16(self : BigInt) -> Int16 {
self.to_int().to_int16()
}
Recommendation
Consider implementing a generic conversion helper or document this as the established pattern for future similar conversions
Reasoning
Both methods follow the identical pattern of converting through Int first. While simple, this pattern should be consistent across all similar conversion methods and the potential limitations should be documented.

Missing edge case documentation for overflow behavior

Category
Correctness
Code Snippet
/// let big = 32768N // 2^15
/// inspect(big.to_int16(), content="-32768") // Overflow to Int16.min_value
Recommendation
Add documentation explaining the two-step conversion process and its implications, especially for BigInt values exceeding Int range
Reasoning
The documentation shows overflow behavior for Int16 but doesn't mention that BigInt values larger than Int.max_value will first overflow during the to_int() conversion, potentially masking the true lower bits that the user might expect.

@coveralls
Copy link
Collaborator
coveralls commented Jul 1, 2025

Pull Request Test Coverage Report for Build 179

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 89.234%

Totals Coverage Status
Change from base Build 175: 0.0%
Covered Lines: 3473
Relevant Lines: 3892

💛 - Coveralls

@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) July 2, 2025 05:49
@peter-jerry-ye peter-jerry-ye merged commit 66df8a8 into moonbitlang:main Jul 2, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0