Skip to content

Conversation

Yu-zh
Copy link
Collaborator

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

StringView is tricky so will do it in a separate PR

Copy link

peter-jerry-ye-code-review bot commented Jan 10, 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. Duplicate Test Cases in panic_test.mbt:

    • The file panic_test.mbt contains duplicate test cases for panic array_as_view_start_index_error and panic array_as_view_end_index_error. These duplicates are redundant and should be removed to avoid confusion and unnecessary test execution. For example:
      test "panic array_as_view_start_index_error" {
        [1, 2, 3][-1:0] |> ignore
      }
      This test case appears twice, and one of them should be removed.
  2. Inconsistent Error Message in arrayview.mbt:

    • The error message in the guard statement was changed from "View start index out of bounds" to "View index out of bounds". While this change is minor, it might be worth ensuring that this change is intentional and consistent with the rest of the codebase. If this change is intentional, it should be documented or communicated to avoid confusion.
  3. Potential Issue with Negative Index Handling:

    • The handling of negative indices in arrayview.mbt and bytesview.mbt has been updated to support negative indices by adding len + index for negative values. However, there is no explicit check to ensure that the resulting index after this adjustment is still within valid bounds. For example:
      let start = if start < 0 { len + start } else { start }
      If start is a large negative number (e.g., -100), len + start could result in a negative number, which might not be caught by the subsequent guard statement. This could lead to unexpected behavior or bugs. It might be worth adding additional checks to ensure that the adjusted indices are non-negative.

These are the key observations from the diff. Addressing these issues will help improve the robustness and clarity of the code.

@coveralls
Copy link
Collaborator

coveralls commented Jan 10, 2025

Pull Request Test Coverage Report for Build 4700

Details

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 82.985%

Totals Coverage Status
Change from base Build 4699: 0.01%
Covered Lines: 4760
Relevant Lines: 5736

💛 - Coveralls

@bobzhang bobzhang force-pushed the negative-slice-index branch from 35ffabf to 3c6788c Compare January 11, 2025 14:11
@bobzhang bobzhang enabled auto-merge (rebase) January 11, 2025 14:11
@bobzhang bobzhang merged commit 1f58bfc into moonbitlang:main Jan 11, 2025
14 checks passed
@Yu-zh Yu-zh deleted the negative-slice-index branch January 11, 2025 15:29
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