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

fix(double): op_mod for subnormal double (#1303) #1367

Merged
merged 4 commits into from
Dec 27, 2024

Conversation

liuly0322
Copy link
Contributor

@liuly0322 liuly0322 commented Dec 25, 2024

Closes #1303

Rewrite op_mod for Double using MUSL implementation (src/math/fmod.c). The origin license of the file is in commit b69f695.

More tests are provided.

Copy link

peter-jerry-ye-code-review bot commented Dec 25, 2024

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

Here are three observations from the provided git diff output:

  1. Inconsistent Pluralization in NOTICE File:

    • The line File float/exp.mbt, float/log.mbt, and double/scalbn.mbt is adapted from was changed to include double/mod_nonjs.mbt, but the verb "is" was not updated to "are" to match the plural subject. The corrected line should be: Files float/exp.mbt, float/log.mbt, double/mod_nonjs.mbt, and double/scalbn.mbt are adapted from.
  2. Potential Issue in mod_nonjs.mbt:

    • In the op_mod function, the line return x * y / (x * y) is used to handle cases where y is zero, NaN, or x is infinity. This expression will always evaluate to 1.0 unless x or y is NaN or infinity, which might not be the intended behavior. It would be clearer and more correct to directly return not_a_number in these cases.
  3. Redundant Test Cases in mod_test.mbt:

    • The test case inspect!(5.75 % 5.75, content="0") is redundant because it tests the same scenario as inspect!(8.0 % 4.0, content="0") and inspect!(15.0 % 3.0, content="0"), where the result of the modulo operation is zero. While it's good to have multiple test cases, they should cover different scenarios to ensure comprehensive testing. Consider adding more diverse test cases instead of repeating similar ones.

These observations highlight areas where improvements can be made for clarity, correctness, and efficiency.

@coveralls
Copy link
Collaborator

coveralls commented Dec 25, 2024

Pull Request Test Coverage Report for Build 4362

Details

  • 22 of 22 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 83.306%

Totals Coverage Status
Change from base Build 4360: 0.2%
Covered Lines: 4651
Relevant Lines: 5583

💛 - Coveralls

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (3)
  • NOTICE: Language not supported
  • double/mod_nonjs.mbt: Language not supported
  • double/mod_test.mbt: Language not supported
Copy link
Contributor

@tonyfettes tonyfettes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bobzhang bobzhang merged commit c1efa9c into moonbitlang:main Dec 27, 2024
13 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.

Double::op_mod on Wasm backend produce JS-inconsistent result when the divisor is in subnormal range
5 participants