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

Support using negative index to take StringView #1466

Merged
merged 4 commits into from
Jan 13, 2025

Conversation

Yu-zh
Copy link
Collaborator

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

Several changes:

  • Given str with n code points, previously String::index(str, i) returns Some if 0 <= i < n. This PR changes this behavior so that it return Some if 0 <= i <= n. This is because the result StringIndex is used for taking StringView where index is allowed to be the length of string.
  • Added String::index_by_rev to search for an index backwards
  • Enhanced StringView::op_as_view so that it can take negative index, and when the index is negative, it uses String::index_by_rev to create the new view.

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 potential issues or observations from the provided git diff output:

  1. Inconsistent Handling of Negative Indices in index_at_rev:

    • In the index_at_rev function, the logic for handling negative indices is not explicitly documented or tested. While the function seems to handle negative indices by counting backward from the end of the string, the behavior is not clearly explained in the documentation. This could lead to confusion for users who expect consistent behavior across all string manipulation functions.
  2. Potential Bug in index_at Function:

    • In the index_at function, the condition if char_count < offset_by || utf16_offset > str_len is used to determine whether to return None. However, the condition utf16_offset > str_len might be incorrect because utf16_offset should never exceed str_len if the function is implemented correctly. This could indicate a logic error or an unnecessary check.
  3. Incomplete Error Handling in op_as_view:

    • In the op_as_view function, the error handling for invalid indices is not fully robust. For example, if index_at or index_at_rev returns None, the function aborts with a generic error message. This could make debugging difficult for users who encounter issues with invalid indices. Adding more descriptive error messages or logging the specific invalid index values could improve the debugging experience.

These issues should be reviewed and addressed to ensure the code is robust, maintainable, and user-friendly.

@coveralls
Copy link
Collaborator

coveralls commented Jan 13, 2025

Pull Request Test Coverage Report for Build 4724

Details

  • 25 of 30 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 82.888%

Changes Missing Coverage Covered Lines Changed/Added Lines %
string/view.mbt 25 30 83.33%
Totals Coverage Status
Change from base Build 4719: -0.01%
Covered Lines: 4805
Relevant Lines: 5797

💛 - Coveralls

@Yu-zh Yu-zh requested a review from bobzhang January 13, 2025 03:44
@bobzhang bobzhang enabled auto-merge (rebase) January 13, 2025 05:44
@bobzhang bobzhang merged commit 365f3f8 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