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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

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 might be inconsistent. The function is designed to handle unsigned integers, so negative values should indeed return 0UL, but this behavior should be clearly documented to avoid confusion.
  2. Redundant Code in double_to_int64_js.mbt:

    • The MyInt64::from_double function was moved from int64_js.mbt to double_to_int64_js.mbt. However, the original function in int64_js.mbt was not removed, leading to potential redundancy. This could cause confusion or errors if both versions are used simultaneously. Ensure that the original function is removed to avoid duplication.
  3. Potential Overflow in to_uint Implementation:

    • In the Double::to_uint function, the condition self >= 4294967295.0 checks if the value is greater than or equal to the maximum UInt value. However, floating-point comparisons can be tricky due to precision issues. Ensure that the comparison is robust and handles edge cases correctly, especially when dealing with values very close to 4294967295.0.

These issues should be addressed to ensure the code is robust, maintainable, and free from potential bugs.

@coveralls
Copy link
Collaborator

coveralls commented Jan 3, 2025

Pull Request Test Coverage Report for Build 4527

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.236%

Totals Coverage Status
Change from base Build 4525: 0.0%
Covered Lines: 4717
Relevant Lines: 5667

💛 - Coveralls

@tonyfettes tonyfettes force-pushed the double-to-uint branch 2 times, most recently from 2f7c852 to cee5afc Compare January 4, 2025 03:52
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
2 participants