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

Merged
merged 1 commit into from
May 30, 2025
Merged

Add Array::windows #2068

merged 1 commit into from
May 30, 2025

Conversation

MINGtoMING
Copy link
Contributor

Add Array::windows.

Copy link

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

Missing input validation for window size parameter

Category
Correctness
Code Snippet
pub fn[T] Array::windows(self : Array[T], size : Int) -> Array[ArrayView[T]] {
let len = self.length() - size + 1
Recommendation
Add validation to check if size > 0 at the start of the function
Reasoning
The documentation states that size must be positive, but the code doesn't enforce this. Negative or zero size could lead to unexpected behavior or runtime errors.

Potential memory pre-allocation optimization

Category
Performance
Code Snippet
let windows = Array::new(capacity=len)
for i in 0..<len {
windows.push(self[i:i + size])
Recommendation
Good use of capacity pre-allocation. Consider adding a comment explaining why len is the optimal capacity.
Reasoning
While the code correctly pre-allocates capacity, documenting this optimization would help maintainers understand the performance consideration.

Test case 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 window size = array length and edge case window size = 0
Reasoning
While current tests cover basic cases, including edge cases like window size equal to array length would improve test coverage and documentation of behavior.

@coveralls
Copy link
Collaborator

coveralls commented May 7, 2025

Pull Request Test Coverage Report for Build 7004

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.01%) to 92.521%

Totals Coverage Status
Change from base Build 7003: 0.01%
Covered Lines: 8746
Relevant Lines: 9453

💛 - 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.

@peter-jerry-ye peter-jerry-ye enabled auto-merge (rebase) May 30, 2025 09:16
@peter-jerry-ye peter-jerry-ye merged commit df2288b into moonbitlang:main May 30, 2025
11 of 12 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.

None yet

4 participants