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): ByteArrayIterator #6938

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

julio4
Copy link
Contributor

@julio4 julio4 commented Dec 27, 2024

Close #6817

Implement iter::IntoIterator<ByteArray> following ByteArrayTrait::len() and ByteArrayTrait::at() -> Option<u8>

Example

let ba: ByteArray = "hello";
let expected = array!['h', 'e', 'l', 'l', 'o'];
let mut i = 0;
for c in ba {
    assert_eq!(c, *expected[i]);
    i += 1;
}

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

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


corelib/src/ops.cairo line 14 at r1 (raw file):

#[allow(unused_imports)]
use range::RangeOp;
pub use range::{Range, RangeTrait};

making it public sounds more sensible for me.

Suggestion:

mod range;
// `RangeOp` is used internally by the compiler.
#[allow(unused_imports)]
use range::RangeOp;
pub use range::{Range, RangeTrait, RangeIterator};

corelib/src/byte_array.cairo line 498 at r1 (raw file):

pub struct ByteArrayIter {
    ba: ByteArray,
    alive: crate::ops::range::RangeIterator<usize>,

alive?

Code quote:

    alive: crate::ops::range::RangeIterator<usize>,

corelib/src/byte_array.cairo line 498 at r1 (raw file):

pub struct ByteArrayIter {
    ba: ByteArray,
    alive: crate::ops::range::RangeIterator<usize>,

add a todo to do an actual efficient implementation (as something more intrusive would obviously be more efficient)

Code quote:

pub struct ByteArrayIter {
    ba: ByteArray,
    alive: crate::ops::range::RangeIterator<usize>,

@julio4
Copy link
Contributor Author

julio4 commented Dec 28, 2024

corelib/src/byte_array.cairo line 498 at r1 (raw file):

Previously, orizi wrote…

alive?

Alive kind of represent the elements that are not yet yielded, so the range of active/valid elements in the iterator lifecycle, with the previous ones being "dead"/consumed.
Maybe there's a better name, feel free to make suggestion.

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 all commit messages.
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @julio4)


corelib/src/byte_array.cairo line 498 at r1 (raw file):

Previously, julio4 (Julio) wrote…

Alive kind of represent the elements that are not yet yielded, so the range of active/valid elements in the iterator lifecycle, with the previous ones being "dead"/consumed.
Maybe there's a better name, feel free to make suggestion.

even current_index sounds better in my mind.

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, 3 unresolved discussions (waiting on @orizi)


corelib/src/byte_array.cairo line 498 at r1 (raw file):

Previously, orizi wrote…

add a todo to do an actual efficient implementation (as something more intrusive would obviously be more efficient)

Done.


corelib/src/byte_array.cairo line 498 at r1 (raw file):

Previously, orizi wrote…

even current_index sounds better in my mind.

Done.


corelib/src/ops.cairo line 14 at r1 (raw file):

Previously, orizi wrote…

making it public sounds more sensible for me.

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.

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


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


corelib/src/test/byte_array_test.cairo line 495 at r2 (raw file):

        i += 1;
    };
    assert_eq!(iter.next(), Option::None);

feels more direct for me.

Suggestion:

    let ba: ByteArray = "hello";
    let mut iter = ba.into_iter();
    assert_eq!(iter.next(), Option::Some('h');
    assert_eq!(iter.next(), Option::Some('e');
    assert_eq!(iter.next(), Option::Some('l');
    assert_eq!(iter.next(), Option::Some('l');
    assert_eq!(iter.next(), Option::Some('o');
    assert_eq!(iter.next(), Option::None);

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, 2 unresolved discussions (waiting on @orizi)


corelib/src/test/byte_array_test.cairo line 495 at r2 (raw file):

Previously, orizi wrote…

feels more direct for me.

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.

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


corelib/src/test/language_features/for_test.cairo line 78 at r3 (raw file):

        i += 1;
    }
}

no need for this test - this is for for tests - not for iterators.

Code quote:

#[test]
fn test_for_loop_ba() {
    let ba: ByteArray = "hello";
    let expected = array!['h', 'e', 'l', 'l', 'o'];
    let mut i = 0;
    for c in ba {
        assert_eq!(c, *expected[i]);
        i += 1;
    }
}

@julio4 julio4 force-pushed the feat/bytearray_iter branch from ccb32b5 to 656cd43 Compare December 29, 2024 13:39
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, 2 unresolved discussions (waiting on @orizi)


corelib/src/test/language_features/for_test.cairo line 78 at r3 (raw file):

Previously, orizi wrote…

no need for this test - this is for for tests - not for iterators.

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


corelib/src/byte_array.cairo line 505 at r3 (raw file):

    type Item = u8;
    /// Advances the iterator and returns the next value. Returns `Option::None` when iteration is
    /// finished.

The trait function is documented, and this isn't adding anything specific to the impl

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


corelib/src/byte_array.cairo line 505 at r3 (raw file):

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

The trait function is documented, and this isn't adding anything specific to the impl

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.

Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (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.

:lgtm:

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

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.

feat: impl IntoIterator on ByteArray
4 participants