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

split for immut/array #1507

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jialunzhang-psu
Copy link
Collaborator

No description provided.

Copy link

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

Looking at the git diff, I can identify the following potential issues:

  1. In Tree::split function, the parameter order is inconsistent with its usage in split_at. In split_at, the call is made as self.tree.split(index, self.shift) but the function is defined as Tree::split[T](self : Tree[T], shift : Int, idx : Int). The parameters shift and idx are in reverse order compared to how they're used.

  2. The Tree::split implementation currently returns (Empty, Empty) which is likely a placeholder/stub implementation. This will cause the split_at function to return empty arrays regardless of the input, which is incorrect behavior.

  3. In the split_at function, there's no bounds checking for the index parameter. This could lead to runtime errors if the index is negative or greater than the array size. It would be safer to add validation for the index parameter.

These issues should be addressed to ensure correct functionality of the array splitting operations.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 4833

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 83.071%

Changes Missing Coverage Covered Lines Changed/Added Lines %
immut/array/array.mbt 0 1 0.0%
immut/array/tree.mbt 0 1 0.0%
Totals Coverage Status
Change from base Build 4831: -0.03%
Covered Lines: 5005
Relevant Lines: 6025

💛 - Coveralls

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.

2 participants