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

refactor parse decimal #1518

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Jan 21, 2025

This PR refactors @strconv.parse_decimal by using pattern matching on StringView to avoid misuses of string indexing.

Copy link

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

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

Looking at the git diff, here are the potential issues I've noticed:

  1. In the new code, there's a typo/syntax error in the last line of parse_decimal_from_view:
d..trim()  // Uses two dots

Should be:

d.trim()   // Should use single dot for method call
  1. The function doesn't have explicit handling for empty strings. While the pattern matching will catch it, it might be clearer to add an explicit check at the start:
guard not(str.length_eq(0)) else { syntax_err!() }
  1. The exponent parsing could potentially overflow since there's no limit check on the exponent value. It would be safer to add a check for maximum exponent size before performing the multiplication:
exp = exp * 10 + (digit.to_int() - '0'.to_int())
// Should check if exp exceeds some maximum value

These suggestions aim to improve the robustness and clarity of the code while preventing potential runtime issues.

@Yu-zh Yu-zh force-pushed the refactor-parse-decimal branch from 5ac75d7 to 4ad903d Compare January 21, 2025 04:52
@coveralls
Copy link
Collaborator

coveralls commented Jan 21, 2025

Pull Request Test Coverage Report for Build 5054

Details

  • 21 of 27 (77.78%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 85.242%

Changes Missing Coverage Covered Lines Changed/Added Lines %
strconv/decimal.mbt 21 27 77.78%
Totals Coverage Status
Change from base Build 5046: 0.06%
Covered Lines: 5239
Relevant Lines: 6146

💛 - Coveralls

@Yu-zh Yu-zh requested a review from bobzhang January 21, 2025 05:04
@bobzhang bobzhang force-pushed the refactor-parse-decimal branch from 4ad903d to af563e0 Compare January 24, 2025 00:45
@bobzhang bobzhang enabled auto-merge January 24, 2025 00:45
@bobzhang bobzhang added this pull request to the merge queue Jan 24, 2025
Merged via the queue into moonbitlang:main with commit 5b7fef5 Jan 24, 2025
14 checks passed
@Yu-zh Yu-zh deleted the refactor-parse-decimal branch January 24, 2025 02:04
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