Skip to content

Conversation

@bobzhang
Copy link
Contributor

@bobzhang bobzhang commented Nov 5, 2025

No description provided.

@peter-jerry-ye-code-review
Copy link

Duplicated character counting logic across multiple helper functions

Category
Maintainability
Code Snippet
Lines 32-78 in stringview.mbt: Three separate functions (count_chars_in_range, has_at_least_n_chars, char_length_equals) contain nearly identical UTF-16 surrogate pair handling logic
Recommendation
Extract the surrogate pair handling into a shared helper function that returns the next index and character increment, then use it in all three functions to eliminate duplication
Reasoning
The current approach violates DRY principle and makes maintenance harder. Any bug fixes or optimizations to surrogate pair handling would need to be applied in three places.

Inconsistent error handling between refactored and original implementations

Category
Correctness
Code Snippet
Lines 65-68 in string.mbt: Original String::char_length had explicit bounds checking with abort(), but the new StringView implementation doesn't show equivalent validation
Recommendation
Ensure StringView::make_view and related functions perform the same bounds validation as the original String::char_length implementation, or document why it's not needed
Reasoning
The original implementation had safety checks that may have been lost during refactoring, potentially introducing runtime errors or undefined behavior.

char_length_equals function has suboptimal early termination logic

Category
Performance
Code Snippet
Lines 76-78 in stringview.mbt: count == len && index == end - checks both conditions even when count != len
Recommendation
Change to index == end && count == len to short-circuit when we haven't reached the end of the string, or restructure the loop to return false immediately when count exceeds len
Reasoning
The current order evaluates both conditions unnecessarily. When count != len, we don't need to check if index == end since the result will be false regardless.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 1783

Details

  • 10 of 30 (33.33%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 89.644%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/stringview.mbt 9 29 31.03%
Files with Coverage Reduction New Missed Lines %
string/string.mbt 2 95.1%
Totals Coverage Status
Change from base Build 1782: -0.2%
Covered Lines: 9989
Relevant Lines: 11143

💛 - Coveralls

@bobzhang bobzhang closed this Nov 20, 2025
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