Skip to content

Conversation

FlyCloudC
Copy link
Contributor

@FlyCloudC FlyCloudC commented Sep 18, 2025

  1. Simplified the overly verbose AI-generated tests.
  2. Refactored parse_inf_nan and parse_number, moving the error handling logic inside them since they are only called in one place, avoiding the need to return the length.
  3. Inline StringView::step leverage contextual information for optimization.
  4. Use a clearer approach to handle positive and negative signs.

@coveralls
Copy link
Collaborator

coveralls commented Sep 18, 2025

Pull Request Test Coverage Report for Build 1292

Details

  • 27 of 30 (90.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 89.459%

Changes Missing Coverage Covered Lines Changed/Added Lines %
strconv/number.mbt 20 23 86.96%
Files with Coverage Reduction New Missed Lines %
strconv/number.mbt 1 91.53%
Totals Coverage Status
Change from base Build 1290: 0.02%
Covered Lines: 9285
Relevant Lines: 10379

💛 - Coveralls

/// Parse the number from the string, returning the number and the length of the parsed string.
fn parse_number(s : StringView) -> (Number, Int)? {
let mut s = s
fn parse_number(s : StringView) -> Number? raise StrConvError {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please state the motivation for the PR and update the corresponding comments in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Copy link

Missing function signature update in parse_scientific call

Category
Correctness
Code Snippet
// Line 76 in number.mbt
if s is ['e' | 'E', .. rest] {
let (new_s, exp_number) = match parse_scientific(rest) {
Some(res) => res
None => return None
}
s = new_s
exponent += exp_number
}
Recommendation
The parse_scientific function was modified to remove the initial step(1) call, but the caller now passes rest instead of the full string. Verify that parse_scientific correctly handles the input without the 'e'/'E' character, or update the function signature accordingly.
Reasoning
The refactoring removed the step(1) call from inside parse_scientific but the caller now strips the 'e'/'E' character before calling it. This could lead to inconsistent behavior if parse_scientific expects to handle the 'e'/'E' character internally.

Potential logic error in guard condition matching

Category
Correctness
Code Snippet
// Line 170 in number.mbt
guard rest
is ([] | ['i' | 'I', 'n' | 'N', 'i' | 'I', 't' | 'T', 'y' | 'Y']) else {
syntax_err()
}
Recommendation
Consider whether this pattern match correctly handles all valid infinity suffix cases. The pattern should match either empty string or exactly 'inity' (case insensitive). Verify this covers all expected inputs and doesn't allow partial matches.
Reasoning
The guard condition uses a complex pattern that needs to exactly match either empty or the full 'inity' suffix. Any deviation or partial match would trigger an error, which may be too strict compared to the original logic that used length-based validation.

Inconsistent error handling pattern between parse functions

Category
Maintainability
Code Snippet
// In parse_double function:
match parse_number(str) {
None => parse_inf_nan(str)
Some(num) => // ...
}

// parse_number returns Option, parse_inf_nan raises exception
Recommendation
Consider making both parse_number and parse_inf_nan follow the same error handling pattern - either both return Option or both raise exceptions. This would make the API more consistent.
Reasoning
Having mixed error handling patterns (Option vs exceptions) in related functions can lead to confusion and makes the code harder to maintain. Consistent error handling patterns improve code readability and reduce cognitive load.

@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) September 23, 2025 03:17
@peter-jerry-ye peter-jerry-ye merged commit 59f6cfe into moonbitlang:main Sep 23, 2025
14 checks passed
@FlyCloudC FlyCloudC deleted the 0918-strconv branch September 23, 2025 04:24
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