Skip to content

Conversation

@giladchase
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@giladchase giladchase marked this pull request as ready for review September 16, 2025 13:18
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from f840c46 to bcf3c7d Compare September 18, 2025 07:11
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from d2487b7 to 235db8b Compare September 18, 2025 07:11
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:

@TomerStarkware reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @orizi)

@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from 235db8b to eeaa0bf Compare September 18, 2025 08:17
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from bcf3c7d to 04b763d Compare September 18, 2025 08:17
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 04b763d to c03411e Compare September 28, 2025 06:43
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch 2 times, most recently from 45abe52 to afd7694 Compare September 28, 2025 06:44
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch 2 times, most recently from 8885cbb to a4243a8 Compare September 28, 2025 08:26
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch 2 times, most recently from 9ab073e to b38e4cc Compare September 28, 2025 12:30
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from a4243a8 to 6769953 Compare September 28, 2025 12:30
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from b38e4cc to 20ea89a Compare September 28, 2025 13:00
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 6769953 to 19ce8b6 Compare September 28, 2025 13:00
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from 20ea89a to bc5dfbf Compare September 29, 2025 06:44
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 19ce8b6 to 4bf4932 Compare September 29, 2025 06:44
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.

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


corelib/src/byte_array.cairo line 703 at r4 (raw file):

    fn at(self: @ByteSpan, index: usize) -> Option<u8> {
        let actual_index = index.checked_add(upcast(self.first_char_start_offset))?;
        let (word_index, index_in_word) = DivRem::div_rem(actual_index, BYTES_IN_BYTES31_NONZERO);

use bounded_int::div_rem and get index_in_word in [0,30] range.
you can use bounded_int::constrain later to decide if it is accessing the lower or upper word.

Code quote:

let (word_index, index_in_word) = DivRem::div_rem(actual_index, BYTES_IN_BYTES31_NONZERO);

corelib/src/byte_array.cairo line 713 at r4 (raw file):

                if word_index == self.data.len() && index_in_word < upcast(self.remainder_len) {
                    // index_in_word is from MSB, we need index from LSB.
                    let index_in_remainder = upcast(self.remainder_len) - 1 - index_in_word;

bounded_int::sub as well.

Code quote:

 let index_in_remainder = upcast(self.remainder_len) - 1 - index_in_word;

@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from bc5dfbf to 50fdc8e Compare September 29, 2025 07:26
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 02d1817 to 826e33e Compare October 20, 2025 08:16
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch 2 times, most recently from 5148fd0 to 2dd37d7 Compare October 20, 2025 09:28
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 826e33e to 1b13525 Compare October 20, 2025 09:28
Copy link
Contributor Author

@giladchase giladchase 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 5 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)


corelib/src/byte_array.cairo line 1014 at r9 (raw file):

Previously, orizi wrote…

should work.

Messes up semantic:

thread '<unnamed>' panicked at crates/cairo-lang-semantic/src/items/constant.rs:839:19:
Variant extract failed: `Extern(ExternFunctionId(f830))` is not of variant `GenericFunctionId::Impl`

When debugging it i switched it the +30 to -30 and it passed semantic (but failed in the logic of course), meaning probably semantic is having problems deciding what type to evaluate this to (maybe it tries to evaluate the type after the addition before taking the Div into account?).
Maybe this is a bug in semantic.

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.

@orizi reviewed all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @giladchase and @TomerStarkware)


corelib/src/byte_array.cairo line 1014 at r9 (raw file):

Previously, giladchase wrote…

Messes up semantic:

thread '<unnamed>' panicked at crates/cairo-lang-semantic/src/items/constant.rs:839:19:
Variant extract failed: `Extern(ExternFunctionId(f830))` is not of variant `GenericFunctionId::Impl`

When debugging it i switched it the +30 to -30 and it passed semantic (but failed in the logic of course), meaning probably semantic is having problems deciding what type to evaluate this to (maybe it tries to evaluate the type after the addition before taking the Div into account?).
Maybe this is a bug in semantic.

lets go with `(Bounded::::MAX / 31 + 1).into(); instead.

@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 1b13525 to ec6fe60 Compare October 20, 2025 12:56
@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from 2dd37d7 to e75d288 Compare October 20, 2025 12:56
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from ec6fe60 to 2a75931 Compare October 20, 2025 13:24
Copy link
Contributor Author

@giladchase giladchase 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 5 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)


corelib/src/byte_array.cairo line 1014 at r9 (raw file):

Previously, orizi wrote…

lets go with `(Bounded::::MAX / 31 + 1).into(); instead.

Nice, 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.

@orizi reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @TomerStarkware)

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.

@orizi reviewed 1 of 1 files at r11.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase force-pushed the gilad/09-14-feat_byte_array_add_slice_ branch from e75d288 to f21ae74 Compare October 21, 2025 07:15
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 2a75931 to d1a4067 Compare October 21, 2025 07:15
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.

@orizi reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

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: all files reviewed, 1 unresolved discussion (waiting on @giladchase)


corelib/src/test/byte_array_test.cairo line 715 at r13 (raw file):

    assert_eq!(span[61], '#', "byte 61 - first in last_word");
    assert_eq!(span.get(62_usize), Some('$'), "byte 62 - last in last_word");
    assert_eq!(span.get(63_usize), None);

remove useless expect.

Code quote:

    assert_eq!(span.get(30_usize), Some('f'), "byte 30 - last of 1nd word");
    assert_eq!(span[31], 'g', "byte 31 - first of 2nd word");
    assert_eq!(span.get(60_usize), Some('9'), "byte 60 - last of 2nd word");
    assert_eq!(span[61], '#', "byte 61 - first in last_word");
    assert_eq!(span.get(62_usize), Some('$'), "byte 62 - last in last_word");
    assert_eq!(span.get(63_usize), None);

@giladchase giladchase changed the base branch from gilad/09-14-feat_byte_array_add_slice_ to main October 21, 2025 08:19
@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from d1a4067 to 9e255e1 Compare October 21, 2025 08:19
@graphite-app
Copy link

graphite-app bot commented Oct 21, 2025

Merge activity

  • Oct 21, 8:19 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.
  • Oct 21, 8:55 AM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
  • Oct 21, 8:55 AM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..

@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch 2 times, most recently from c8b0b6b to 81d99a8 Compare October 21, 2025 10:12
Copy link
Contributor Author

@giladchase giladchase 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 @orizi)


corelib/src/test/byte_array_test.cairo line 715 at r13 (raw file):

Previously, orizi wrote…

remove useless expect.

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:

@orizi reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase force-pushed the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch from 81d99a8 to 7ce15d9 Compare October 22, 2025 06:08
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.

@orizi reviewed 2 of 2 files at r15, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase added this pull request to the merge queue Oct 22, 2025
Merged via the queue into main with commit 6dde184 Oct 22, 2025
54 checks passed
@orizi orizi deleted the gilad/09-16-feat_byte_array_add_at_and_index_to_bytespan_ branch October 22, 2025 08:17
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.

5 participants