Skip to content

feat(iter): add mapi #2011

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
Apr 27, 2025
Merged

feat(iter): add mapi #2011

merged 1 commit into from
Apr 27, 2025

Conversation

peter-jerry-ye
Copy link
Collaborator

Related #1866

Note that we may want a labelled argument offset or something similar in the future to avoid computing the addition every time.

@peter-jerry-ye peter-jerry-ye requested a review from hackwaly April 27, 2025 06:10
Copy link

Counter initialization and increment could be optimized

Category
Performance
Code Snippet
let mut i = -1
i = i + 1
Recommendation
Consider using let mut i = 0 for initialization and the increment operator i += 1
Reasoning
Starting from -1 and using addition is less intuitive and potentially less performant than starting from 0 and using the increment operator. The current implementation also requires an additional operation on the first iteration.

Function documentation could be more detailed with examples

Category
Maintainability
Code Snippet
///|
/// Transforms the elements of the iterator using a mapping function.
Recommendation
Add example usage in documentation showing common use cases, similar to other iterator methods
Reasoning
Examples in documentation help users understand the intended usage and make the API more accessible. The current documentation, while technically correct, could be more helpful with concrete examples.

Consider adding bounds check documentation

Category
Maintainability
Code Snippet
pub fn Iter::mapi[T, R](self : Iter[T], f : (Int, T) -> R) -> Iter[R]
Recommendation
Add documentation about the index bounds (starting from 0) and any limitations on the maximum index value
Reasoning
Users should be aware of the index behavior, especially for very large iterators where index values might be important. This helps prevent potential misuse of the API.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 6459

Details

  • 3 of 3 (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.673%

Totals Coverage Status
Change from base Build 6455: 0.003%
Covered Lines: 6122
Relevant Lines: 6606

💛 - Coveralls

@hackwaly
Copy link
Contributor

There's both map_with_index and mapi in core. We should choose one to stick.

@peter-jerry-ye peter-jerry-ye merged commit 8efd1db into main Apr 27, 2025
12 checks passed
@peter-jerry-ye peter-jerry-ye deleted the zihang/iter-mapi branch April 27, 2025 08:14
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