Skip to content

Add Array::windows #2068

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Array::windows #2068

wants to merge 2 commits into from

Conversation

MINGtoMING
Copy link
Contributor

Add Array::windows.

Copy link

peter-jerry-ye-code-review bot commented May 7, 2025

No validation for negative window size

Category
Correctness
Code Snippet
pub fn Array::windows[T](self : Array[T], size : Int) -> Array[ArrayView[T]]
Recommendation
Add validation to handle negative window sizes by either throwing an error or returning an empty array:

if size <= 0 {
  return []
}

Reasoning
The documentation states size must be positive, but the implementation doesn't enforce this. Negative sizes could cause unexpected behavior or runtime errors.

Test cases could be more comprehensive

Category
Maintainability
Code Snippet
test "array_windows" {
let arr = [1, 2, 3, 4, 5, 6]
let windows = arr.windows(11)
...
}
Recommendation
Add test cases for:

  • Empty input array
  • Window size of 0
  • Negative window size
  • Window size equal to array length
    Reasoning
    Current tests don't cover all edge cases that could reveal potential issues. More comprehensive testing ensures robustness.
Potential memory optimization for large arrays

Category
Performance
Code Snippet
let windows = Array::new(capacity=len)
for i in 0..<len {
windows.push(self[i:i + size])
}
Recommendation
Consider implementing lazy evaluation or iterator pattern for large arrays where all windows aren't needed immediately
Reasoning
Current implementation eagerly creates all window slices, which could be memory-intensive for large arrays when only a few windows are actually needed

@coveralls
Copy link
Collaborator

coveralls commented May 7, 2025

Pull Request Test Coverage Report for Build 6907

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 92.457%

Totals Coverage Status
Change from base Build 6906: 0.003%
Covered Lines: 8691
Relevant Lines: 9400

💛 - Coveralls

@FlyCloudC
Copy link
Contributor

How about return an Array[ArrayView[T]]?

@MINGtoMING
Copy link
Contributor Author

MINGtoMING commented May 8, 2025

I agree. Array::windows is typically used in read-only scenarios as well. However, the func interface differs from existing chunks/chunk_by impl, so perhaps they need to be adjusted together.

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