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): iter::adapters::Filter #7152

Merged
merged 3 commits into from
Jan 29, 2025

Conversation

julio4
Copy link
Contributor

@julio4 julio4 commented Jan 23, 2025

Stacked on #7151

Creates an iterator which uses a closure to determine if an element should be yielded.

Given an element the closure must return true or false. The returned iterator will yield only the elements for which the closure returns true.

Example

let a = array![0_u32, 1, 2];

let mut iter = a.into_iter().filter(|x| x > 0);

assert_eq!(iter.next(), Option::Some(1));
assert_eq!(iter.next(), Option::Some(2));
assert_eq!(iter.next(), Option::None);

@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 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @julio4)


corelib/src/iter/adapters/filter.cairo line 32 at r1 (raw file):

    fn next(ref self: Filter<I, P>) -> Option<Self::Item> {
        self.iter.find(self.predicate)
    }

Suggestion:

pub impl FilterIterator<
    I,
    P,
    impl TIter: Iterator<I>,
    +core::ops::Fn<P, (TIter::Item,)>[Output: bool],
    +Destruct<I>,
    +Destruct<P>,
    +Destruct<TIter::Item>,
> of Iterator<Filter<I, P>> {
    type Item = TIter::Item;
    fn next(ref self: Filter<I, P>) -> Option<Self::Item> {
        self.iter.find(self.predicate)
    }

@julio4 julio4 force-pushed the feat/iter_adapter_filter branch from b3a6c4a to bfe7008 Compare January 27, 2025 21:21
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: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/iter/adapters/filter.cairo line 32 at r1 (raw file):

    fn next(ref self: Filter<I, P>) -> Option<Self::Item> {
        self.iter.find(self.predicate)
    }

I'm not sure if it's possible to not copy the predicate closure.

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 4 files at r2, all commit messages.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @julio4 and @TomerStarkware)


corelib/src/iter/adapters/filter.cairo line 32 at r1 (raw file):

Previously, julio4 (Julio) wrote…

I'm not sure if it's possible to not copy the predicate closure.

It is probably ok for this case/
@TomerStarkware ?


corelib/src/iter/adapters/filter.cairo line 18 at r2 (raw file):

}

pub impl FilterIterator<

Suggestion:

impl FilterIterator<

Copy link
Collaborator

@TomerStarkware TomerStarkware 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: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @julio4, @orizi, and @TomerStarkware)


corelib/src/iter/adapters/filter.cairo line 32 at r1 (raw file):

Previously, orizi wrote…

It is probably ok for this case/
@TomerStarkware ?

self.iter.find(@self.predicate)
should work without +Copy

@julio4 julio4 force-pushed the feat/iter_adapter_filter branch from bfe7008 to c8fee23 Compare January 28, 2025 09:40
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: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)


corelib/src/iter/adapters/filter.cairo line 32 at r1 (raw file):

Previously, TomerStarkware wrote…

self.iter.find(@self.predicate)
should work without +Copy

True, I didn't know that we could pass closure as snapshot like that. Done!


corelib/src/iter/adapters/filter.cairo line 18 at r2 (raw file):

}

pub impl FilterIterator<

Done.

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 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (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.

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


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

Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@orizi orizi added this pull request to the merge queue Jan 29, 2025
Merged via the queue into starkware-libs:main with commit 6914ec6 Jan 29, 2025
47 checks passed
@julio4 julio4 deleted the feat/iter_adapter_filter branch January 29, 2025 12:46
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