Skip to content

Conversation

tonyfettes
Copy link
Contributor

No description provided.

@coveralls
Copy link
Collaborator

coveralls commented Sep 18, 2025

Pull Request Test Coverage Report for Build 1284

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 89.615%

Totals Coverage Status
Change from base Build 1283: 0.006%
Covered Lines: 9302
Relevant Lines: 10380

💛 - Coveralls

@tonyfettes tonyfettes force-pushed the haoxiang/json-parse-string-view branch from 74bc284 to 0bec4c8 Compare September 18, 2025 04:01
Copy link

peter-jerry-ye-code-review bot commented Sep 18, 2025

Potential bounds checking issue in string operations

Category
Correctness
Code Snippet
buf.write_substring(
ctx.input.data(),
ctx.input.start_offset() + start,
end - start,
)
Recommendation
Add bounds validation before calling write_substring to ensure start_offset() + start doesn't exceed the underlying data bounds, or use a safer method like ctx.input.view(start_offset=start, end_offset=end).to_string()
Reasoning
The manual offset calculation could lead to out-of-bounds access if the StringView's start_offset and the local start don't align properly with the underlying data

Inconsistent StringView to String conversion pattern

Category
Maintainability
Code Snippet
return Number(n, repr.map(_.to_string()))
Recommendation
Consider creating a helper function for StringView to String conversion to centralize this logic, such as fn optional_stringview_to_string(sv: StringView?) -> String?
Reasoning
The pattern .map(_.to_string()) is repeated 4 times in the same file, creating maintenance overhead and potential for inconsistency

Unnecessary string conversion in number parsing

Category
Performance
Code Snippet
let s = ctx.input.view(start_offset=start, end_offset=end)
if !s.contains(".") && !s.contains("e") && !s.contains("E")
Recommendation
Perform character checking directly on the StringView without creating intermediate views, or cache the view creation if it's used multiple times
Reasoning
Creating a StringView for simple character existence checks may be less efficient than iterating through the original input directly, especially for short number strings

@tonyfettes tonyfettes force-pushed the haoxiang/json-parse-string-view branch from 0bec4c8 to 3fc0815 Compare September 18, 2025 05:48
@peter-jerry-ye peter-jerry-ye force-pushed the haoxiang/json-parse-string-view branch from 32d7ed7 to 2f2edc4 Compare September 22, 2025 09:36
@tonyfettes tonyfettes force-pushed the haoxiang/json-parse-string-view branch 2 times, most recently from 7f1d6a4 to 6108af7 Compare September 26, 2025 04:32
@peter-jerry-ye peter-jerry-ye force-pushed the haoxiang/json-parse-string-view branch from 6108af7 to 9c5f3fd Compare September 26, 2025 07:51
@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) September 26, 2025 07:52
@peter-jerry-ye peter-jerry-ye merged commit bd801f2 into moonbitlang:main Sep 26, 2025
13 of 14 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.

3 participants