Skip to content
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

Add Double::to_uint and Double::to_uint64 #1419

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

tonyfettes
Copy link
Contributor

closes #1416

Copy link

peter-jerry-ye-code-review bot commented Jan 3, 2025

‼️ This code review is generated by a bot. Please verify the content before trusting it.

Here are three potential issues or observations from the provided git diff output:

  1. Inconsistent Handling of Negative Values in to_uint64:

    • In the Double::to_uint64 function, negative values are explicitly handled by returning 0UL. However, the documentation and implementation in double_to_int64_js.mbt and double_to_int64_native.mbt do not clearly explain why negative values are treated this way. This could lead to confusion for users expecting a different behavior (e.g., wrapping around or returning an error). Consider adding a note in the documentation to clarify this design decision.
  2. Redundant Code in int64_js.mbt:

    • The MyInt64::from_double function was removed from int64_js.mbt but was re-added in double_to_int64_js.mbt. This duplication could lead to maintenance issues if the logic needs to be updated in the future. Consider consolidating the implementation in a single location to avoid redundancy.
  3. Potential Overflow Risk in to_uint:

    • In the Double::to_uint function, the condition self >= 4294967295.0 is used to check if the value exceeds the maximum UInt value. However, floating-point comparisons can be tricky due to precision issues. For example, a value like 4294967295.0000001 might not be correctly identified as exceeding the limit. Consider using a safer comparison method or adding a comment to explain the potential edge cases.

These observations are intended to help improve the code's clarity, maintainability, and robustness.

@coveralls
Copy link
Collaborator

coveralls commented Jan 3, 2025

Pull Request Test Coverage Report for Build 4598

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

Totals Coverage Status
Change from base Build 4596: 0.0%
Covered Lines: 4727
Relevant Lines: 5675

💛 - Coveralls

@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) January 8, 2025 08:31
@peter-jerry-ye peter-jerry-ye merged commit 77f964b into moonbitlang:main Jan 8, 2025
14 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.

Add Double::to_uint and Double::to_uint64
3 participants