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

provide length_eq and length_ge for StringView #1467

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

Yu-zh
Copy link
Collaborator

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

Because the length method for StringView has O(n) time complexity, we provide two auxiliary methods length_eq and length_ge which are fast when the target length is small.

@Yu-zh Yu-zh requested a review from bobzhang January 13, 2025 03:44
Copy link

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

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

Here are three observations from the provided git diff output:

  1. Typo in Test Names:

    • In the test cases "stingview length_eq" and "stingview length_ge", the word "stingview" is misspelled. It should be "stringview" to match the naming convention used elsewhere in the codebase. This typo could lead to confusion or issues when searching for or referencing these tests.
  2. Potential Logic Issue in length_eq:

    • The length_eq function checks if the length of the view is equal to the given length. However, the logic in the else block (self_len == len && index == self.end) might not correctly handle cases where the view contains surrogate pairs. Specifically, if the loop exits early due to index < self.end being false, but self_len < len is still true, the function might incorrectly return false even if the lengths match. This could lead to incorrect behavior when dealing with strings containing surrogate pairs.
  3. Redundant Logic in length_ge:

    • The length_ge function checks if the length of the view is greater than or equal to the given length. However, the loop condition index < self.end && self_len < len ensures that the loop exits as soon as self_len reaches len. This means the else block (self_len >= len) will always be true if the loop exits due to self_len < len being false. This redundancy could be simplified by directly returning true after the loop exits, as the condition self_len >= len is already guaranteed by the loop's exit condition.

These issues should be addressed to ensure the correctness and maintainability of the code.

@coveralls
Copy link
Collaborator

coveralls commented Jan 13, 2025

Pull Request Test Coverage Report for Build 4726

Details

  • 10 of 16 (62.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.06%) to 82.832%

Changes Missing Coverage Covered Lines Changed/Added Lines %
string/view.mbt 10 16 62.5%
Totals Coverage Status
Change from base Build 4725: -0.06%
Covered Lines: 4815
Relevant Lines: 5813

💛 - Coveralls

@bobzhang bobzhang force-pushed the string-view-length-api branch from cfefa16 to a908b84 Compare January 13, 2025 05:47
@bobzhang bobzhang enabled auto-merge (rebase) January 13, 2025 05:49
@bobzhang bobzhang merged commit 9cf1483 into moonbitlang:main Jan 13, 2025
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