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

feat(corelib): Iterator::enumerate #7048

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

julio4
Copy link
Contributor

@julio4 julio4 commented Jan 11, 2025

Creates an iterator which gives the current iteration count as well as the next value.

The iterator returned yields pairs (i, val), where i is the current index of iteration and val is the value returned by the iterator.

enumerate() keeps its count as a usize.

Examples

let a = array!['a', 'b', 'c'];

let mut iter = a.into_iter().enumerate();

assert_eq!(iter.next(), Option::Some((0, 'a')));
assert_eq!(iter.next(), Option::Some((1, 'b')));
assert_eq!(iter.next(), Option::Some((2, 'c')));
assert_eq!(iter.next(), Option::None);

Overflow Behavior

The method does no guarding against overflows, so enumerating more than Bounded::<usize>::MAX elements either produces the wrong result or panics.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @julio4)


a discussion (no related file):
@gilbens-starkware for 2nd eye.


corelib/src/iter/traits/iterator.cairo line 44 at r1 (raw file):

    /// let a = array!['a', 'b', 'c'];
    ///
    /// let mut iter = a.into_iter().enumerate();

Suggestion:

    /// let mut iter = array!['a', 'b', 'c'].into_iter().enumerate();

corelib/src/test/iter_test.cairo line 5 at r1 (raw file):

    let a = array!['a', 'b', 'c'];

    let mut iter = a.into_iter().enumerate();

Suggestion:

    let mut iter = array!['a', 'b', 'c'].into_iter().enumerate();

Copy link
Contributor Author

@julio4 julio4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/iter/traits/iterator.cairo line 44 at r1 (raw file):

    /// let a = array!['a', 'b', 'c'];
    ///
    /// let mut iter = a.into_iter().enumerate();

Done.


corelib/src/test/iter_test.cairo line 5 at r1 (raw file):

    let a = array!['a', 'b', 'c'];

    let mut iter = a.into_iter().enumerate();

Done.

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @julio4 and @orizi)


corelib/src/iter/adapters/enumerate.cairo line 28 at r2 (raw file):

    /// The method does no guarding against overflows, so enumerating more than
    /// `Bounded::<usize>::MAX` elements either produces the wrong result or panics. If
    /// debug assertions are enabled, a panic is guaranteed.

We always panic on overflow.

Code quote:

    /// `Bounded::<usize>::MAX` elements either produces the wrong result or panics. If
    /// debug assertions are enabled, a panic is guaranteed.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @julio4)

Copy link
Contributor Author

@julio4 julio4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)


corelib/src/iter/adapters/enumerate.cairo line 28 at r2 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

We always panic on overflow.

Done.

@julio4 julio4 force-pushed the feat/iter_adapter_enumerate branch from 2a49c71 to 5b4800b Compare January 13, 2025 17:57
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @julio4)


corelib/src/iter/traits/iterator.cairo line 113 at r3 (raw file):

    /// The method does no guarding against overflows, so enumerating more than
    /// [`Bounded::<usize>::MAX`] elements either produces the wrong result or panics.
    ///

Same here, also, not sure if the panics section is still needed as this will simply repeat itself.

Code quote:

    /// The method does no guarding against overflows, so enumerating more than
    /// [`Bounded::<usize>::MAX`] elements either produces the wrong result or panics.
    ///

@julio4 julio4 force-pushed the feat/iter_adapter_enumerate branch from 5b4800b to 8502a5b Compare January 15, 2025 09:21
@julio4 julio4 force-pushed the feat/iter_adapter_enumerate branch from 8502a5b to 6622314 Compare January 15, 2025 09:23
Copy link
Contributor Author

@julio4 julio4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)


corelib/src/iter/traits/iterator.cairo line 113 at r3 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Same here, also, not sure if the panics section is still needed as this will simply repeat itself.

Done.
I kept the panic section, even if it's slightly redundant at least it's clear that this can panic

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 6 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @julio4)

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @julio4)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @julio4)

@orizi orizi added this pull request to the merge queue Jan 15, 2025
Merged via the queue into starkware-libs:main with commit 0ec6865 Jan 15, 2025
47 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.

4 participants