feat(corelib): Iterator::take

[cairolover]

No description provided.

[reviewable-StarkWare]

This change is 

[orizi]

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @cairolover)

---

corelib/src/iter/adapters/take.cairo line 11 at r1 (raw file):

pub struct Take<I> {
    pub iter: I,
    pub n: usize,

Suggestion:

pub struct Take<I> {
    iter: I,
    n: usize,

---

corelib/src/iter/adapters/take.cairo line 36 at r1 (raw file):

        if self.n > n {
            self.n -= 1;
            Iterator::nth(ref self.iter, n)

and add a test validating this.

Suggestion:

        if self.n > n {
            self.n -= n;
            Iterator::nth(ref self.iter, n)

---

corelib/src/iter/adapters/take.cairo line 43 at r1 (raw file):

                    Some(_item) => {},
                    None => {},
                }

won't this work?

Suggestion:

                let _ = self.iter.advance(self.n);

[cairolover]

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

---

corelib/src/iter/adapters/take.cairo line 36 at r1 (raw file):

Previously, orizi wrote…

and add a test validating this.

Done. Added test. The correct one actually is -= n-1;

---

corelib/src/iter/adapters/take.cairo line 43 at r1 (raw file):

Previously, orizi wrote…

won't this work?

Done. It works indeed

---

corelib/src/iter/adapters/take.cairo line 11 at r1 (raw file):

pub struct Take<I> {
    pub iter: I,
    pub n: usize,

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion

---

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

[orizi]

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @cairolover)

---

corelib/src/test/iter_test.cairo line 132 at r2 (raw file):

    // Test when n = 0
    let mut iter = array![1, 2, 3].into_iter().take(0);
    assert_eq!(iter.nth(0), None);

please add test cases where all used numbers are larger than 1 as well - just to ensure some more edge cases.

Code quote:

    // Test when n > requested nth
    let mut iter = array![1, 2, 3].into_iter().take(2);
    assert_eq!(iter.nth(1), Some(2));
    assert_eq!(iter.next(), None);

    // Test when n > 0 but not enough elements
    let mut iter = array![1, 2].into_iter().take(2);
    assert_eq!(iter.nth(1), Some(2));
    assert_eq!(iter.nth(0), None);

    // Test when n = 0
    let mut iter = array![1, 2, 3].into_iter().take(0);
    assert_eq!(iter.nth(0), None);

---

corelib/src/test/iter_test.cairo line 144 at r2 (raw file):

    // Test when advancing by more than available elements
    let mut iter = array![1, 2].into_iter().take(2);
    assert_eq!(iter.advance_by(3), Err(1_usize.try_into().unwrap()));

Suggestion:

    assert_eq!(iter.advance_by(3), Err(1));

---

corelib/src/test/iter_test.cairo line 149 at r2 (raw file):

    // Test when n = 0
    let mut iter = array![1, 2, 3].into_iter().take(0);
    assert_eq!(iter.advance_by(1), Err(1_usize.try_into().unwrap()));

Suggestion:

    assert_eq!(iter.advance_by(1), Err(1));

[orizi]

Reviewed 1 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @cairolover)

---

corelib/src/iter/adapters/take.cairo line 61 at r2 (raw file):

            Some(nz) => Err(nz),
            None => Ok(()),
        }

compare maybe - not sure if any better.

Suggestion:

        if self.n >= n {
            self.n -= n;
            match self.iter.advance_by(n) {
                Ok(_) => Ok(()),
                Err(rem) => {
                    self.n += rem.into();
                    rem
                },
            }
        } else {
            self.n = match self.iter.advance_by(self.n) {
                Ok(_) => 0,
                Err(rem) => rem.into(),
            };
            let maybe_nz: Option<NonZero<usize>> = (n - self.n).try_into();
            match maybe_nz {
                Some(nz) => Err(nz),
                // Unreachable - but reducing the possible code size.
                None => Ok(()),
            }
        }

[cairolover]

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

---

corelib/src/iter/adapters/take.cairo line 61 at r2 (raw file):

Previously, orizi wrote…

compare maybe - not sure if any better.

prev: (gas usage est.: 36370)
after: (gas usage est.: 31450)

---

corelib/src/test/iter_test.cairo line 132 at r2 (raw file):

Previously, orizi wrote…

please add test cases where all used numbers are larger than 1 as well - just to ensure some more edge cases.

Done

---

corelib/src/test/iter_test.cairo line 144 at r2 (raw file):

    // Test when advancing by more than available elements
    let mut iter = array![1, 2].into_iter().take(2);
    assert_eq!(iter.advance_by(3), Err(1_usize.try_into().unwrap()));

Done.

---

corelib/src/test/iter_test.cairo line 149 at r2 (raw file):

    // Test when n = 0
    let mut iter = array![1, 2, 3].into_iter().take(0);
    assert_eq!(iter.advance_by(1), Err(1_usize.try_into().unwrap()));

Done.

[cairolover]

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

---

corelib/src/iter/adapters/take.cairo line 61 at r2 (raw file):

Previously, cairolover (cairolover) wrote…

prev: (gas usage est.: 36370)
after: (gas usage est.: 31450)

Done (slight modifications - instruction order needed to be changed.)

[orizi]

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

---

corelib/src/iter/adapters/take.cairo line 62 at r3 (raw file):

        } else {
            let maybe_nz: Option<NonZero<usize>> = (n - self.n).try_into();
            let _n = self.iter.advance_by(n - self.n);

no?

Suggestion:

            let _n = self.iter.advance_by(self.n);

[cairolover]

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @orizi)

---

corelib/src/iter/adapters/take.cairo line 62 at r3 (raw file):

Previously, orizi wrote…

no?

right, sorry, this was not caught up by the tests because setting n=0 would make any method on take return None. (and not use the inner iterator).

Thus, in this context, I'm wondering: should we even advance the inner iterator, considering that it would be a wasted operation?

[orizi]

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

---

corelib/src/iter/adapters/take.cairo line 62 at r3 (raw file):

Previously, cairolover (cairolover) wrote…

right, sorry, this was not caught up by the tests because setting n=0 would make any method on take return None. (and not use the inner iterator).

Thus, in this context, I'm wondering: should we even advance the inner iterator, considering that it would be a wasted operation?

yes - as if the iterator has any side effects they must be called.

currently - the only way i think i can do this is using starknet syscalls or prints during the iterator implementation (in some Map-iterator adaptor)

---

corelib/src/iter/adapters/take.cairo line 61 at r4 (raw file):

            }
        } else {
            let maybe_nz: Option<NonZero<usize>> = (n - self.n).try_into();

is this the actual correct value?

what if the iterator internally ends after self.n - 2 - you should return n - self.n + 2

Code quote:

            let maybe_nz: Option<NonZero<usize>> = (n - self.n).try_into();

[cairolover]

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @orizi)

---

corelib/src/iter/adapters/take.cairo line 62 at r3 (raw file):

Previously, orizi wrote…

yes - as if the iterator has any side effects they must be called.

currently - the only way i think i can do this is using starknet syscalls or prints during the iterator implementation (in some Map-iterator adaptor)

Re-wrote the function, keeping this in mind:

• Even if the internal iterator is exhausted,self.n is not necessarily zero
• If n > self.n then
  a. if the internal iterator is can be advanced, self.n is thus zero, and the remaining part is n - self.n
  b. if the internal iterator overflows, self.n is decreased by the actual consumed amount, and the remaining part is n - consumed

Added test cases for each branch

[orizi]

Reviewed all commit messages.
Reviewable status: 2 of 4 files reviewed, all discussions resolved

---

corelib/src/iter/adapters/take.cairo line 47 at r6 (raw file):

    #[inline]
    fn advance_by<+Destruct<Take<I>>, +Destruct<Self::Item>>(

@gilbens-starkware try to double check this as well.

[orizi]

Reviewed 1 of 2 files at r5.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion

---

corelib/src/iter/adapters/take.cairo line 70 at r6 (raw file):

                    Err((n - (available - rem_nz.into())).try_into().unwrap())
                },
            }

Suggestion:

            let inner_taken = match self.iter.advance_by(available) {
                Ok(_) => {
                    self.n = 0;
                    available
                },
                Err(inner_untaken) => {
                    self.n = inner_untaken.into();
                    available - self.n
                },
            }
            match (n - inner_taken).try_into() {
                Some(nz) => Err(nz),
                // Can't actually happen - but preventing the `unwrap` generated code.
                None => Ok(()),
            }