Skip to content

Conversation

Yu-zh
Copy link
Collaborator

@Yu-zh Yu-zh commented Sep 9, 2025

This PR implements taking view from FixedArray.

Solves #1626

Copy link

peter-jerry-ye-code-review bot commented Sep 9, 2025

Unsafe cast function lacks proper type safety documentation

Category
Correctness
Code Snippet
fn[T] unsafe_cast_fixedarray_to_uninitializedarray(
arr : FixedArray[T],
) -> UninitializedArray[T] = "%identity"
Recommendation
Add comprehensive documentation explaining the safety contract and invariants that must be maintained when using this function. Consider adding a comment about why this cast is safe in this specific context.
Reasoning
Unsafe operations should be thoroughly documented to prevent misuse. The function name has a typo ('Unitialized' should be 'Uninitialized') and lacks documentation about when it's safe to use.

Duplicated bounds checking logic between ArrayView::sub and FixedArray::sub

Category
Maintainability
Code Snippet
let end = match end {
None => len
Some(end) => if end < 0 { len + end } else { end }
}
let start = if start < 0 { len + start } else { start }
guard start >= 0 && start <= end && end <= len else {
abort("View index out of bounds")
}
Recommendation
Extract the bounds checking and negative index handling logic into a shared helper function that both ArrayView::sub and FixedArray::sub can use to reduce code duplication.
Reasoning
The exact same bounds checking logic appears in both functions, violating the DRY principle and making maintenance harder. A shared helper would ensure consistent behavior and reduce the chance of bugs.

Inconsistent error messages between similar functions

Category
Correctness
Code Snippet
guard start >= 0 && start <= end && end <= len else {
abort("View index out of bounds")
}
Recommendation
Use the same error message format as ArrayView::sub or create a standardized error message function to ensure consistency across the codebase.
Reasoning
Different error messages for the same type of error can confuse users and make debugging harder. Consistent error messaging improves user experience and maintainability.

@Yu-zh Yu-zh marked this pull request as ready for review September 15, 2025 09:34
@Yu-zh Yu-zh marked this pull request as draft September 16, 2025 02:02
@Yu-zh Yu-zh force-pushed the fixedarray-take-view branch from 6d2b14b to 4207fb2 Compare September 16, 2025 06:33
@Yu-zh Yu-zh marked this pull request as ready for review September 16, 2025 06:33
@coveralls
Copy link
Collaborator

coveralls commented Sep 16, 2025

Pull Request Test Coverage Report for Build 1229

Details

  • 7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.002%) to 89.646%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/arrayview.mbt 7 8 87.5%
Totals Coverage Status
Change from base Build 1228: -0.002%
Covered Lines: 9299
Relevant Lines: 10373

💛 - Coveralls

@peter-jerry-ye
Copy link
Collaborator

When will ArrayView become immutable?

@Yu-zh
Copy link
Collaborator Author

Yu-zh commented Sep 17, 2025

When will ArrayView become immutable?

The op_set method is already deprecated

@bobzhang bobzhang force-pushed the fixedarray-take-view branch from 85d7be9 to cc1d038 Compare September 17, 2025 09:19
@bobzhang bobzhang enabled auto-merge (rebase) September 17, 2025 09:21
@bobzhang bobzhang merged commit 8459084 into main Sep 17, 2025
15 of 18 checks passed
@bobzhang bobzhang deleted the fixedarray-take-view branch September 17, 2025 09:23
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.

4 participants