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

use zero instead of literal 0.0 #1558

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tonyfettes
Copy link
Contributor

@tonyfettes tonyfettes commented Jan 23, 2025

In CI run:

https://github.com/moonbitlang/core/actions/runs/12911267303/job/36004981375

The CL compiler on Windows will raise a fatal error when there is division by literal 0.0, even if the literal is in type double.

This PR use named variables in double package to replace these usage.

Copy link

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

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

⚠️ Potential Test Failures Due to Incorrect Expected Values
  • Category: Correctness
  • Code Snippet: In float/round_js.mbt, lines like inspect!((3.7 : Float).trunc(), content="3")
  • Recommendation: Revert changes where float results (e.g., 3.0) are compared to integer strings ("3"), unless the language automatically truncates decimal zeros.
  • Reasoning: If the actual result is a float (e.g., 3.0), expecting "3" without the decimal part may cause test failures. This change was not explained in the PR message and seems unrelated to the division-by-zero fix.
⚠️ Duplicated Variable Declaration Across Tests
  • Category: Maintainability
  • Code Snippet: Repeated let zero = 0.0 in multiple test blocks (e.g., double_to_int_test.mbt, double_to_int64_test.mbt)
  • Recommendation: Consider defining a module-level constant or helper function to avoid redundancy.
  • Reasoning: Redundant declarations increase maintenance overhead. If the initialization needs to change (e.g., to 0.0f), every test must be updated manually.
💡 Consider Descriptive Variable Name
  • Category: Style Guide Compliance
  • Code Snippet: let zero = 0.0 in test blocks
  • Recommendation: Use a more descriptive name like divisor_zero to clarify intent.
  • Reasoning: While zero is concise, divisor_zero explicitly indicates the variable's role in division, improving readability for future maintainers.

@tonyfettes tonyfettes force-pushed the replace-float-0-with-zero branch from 1607b34 to 124cbc2 Compare January 23, 2025 03:21
@coveralls
Copy link
Collaborator

coveralls commented Jan 23, 2025

Pull Request Test Coverage Report for Build 5053

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

Totals Coverage Status
Change from base Build 5046: 0.0%
Covered Lines: 5233
Relevant Lines: 6143

💛 - Coveralls

@tonyfettes tonyfettes force-pushed the replace-float-0-with-zero branch 4 times, most recently from 3768dd5 to d05f890 Compare January 23, 2025 04:51
@tonyfettes tonyfettes marked this pull request as draft January 23, 2025 09:58
This commit replaces:

- `0.0 / 0.0` (NaN)
- `1.0 / 0.0` (Infinity)
- `-1.0 / 0.0` (-Infinity)

with their corresponding named variables in
double package, i.e.,

- `@double.not_a_number`
- `@double.infinity`
- `@double.neg_infinity`
@tonyfettes tonyfettes force-pushed the replace-float-0-with-zero branch from d05f890 to 7749b6b Compare January 23, 2025 10:10
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.

2 participants