feat(byte_array): add ByteSpan::to_byte_array

[giladchase]

When slicing bytespans (to-be-implemented), the end-offset is trimmed by shifting the word to the
right.
The left offset, however, will be trimmed lazily only if the `ByteSpan` is casted into a `ByteArray`.

Note: Lazily removing the end-offset will require saving an additional field, `end_offset` in
`ByteSpan`, due to how strings are represented inside felt252 (the first byte is the msb of the
word). In other words, we cannot just reduce the remainder_len, because then it'd be impossible to
know how much to trim off from the remainder word at `to_byte_array`.

[reviewable-StarkWare]

This change is 

[giladchase]

• feat(ops): add get, SliceIndex and impl for ByteSpan #8417 : 2 dependent PRs (#8427 , #8511 )
• feat(byte_array): add ByteSpan::to_byte_array #8416 👈 (View in Graphite)
• refactor(byte_array): extract ByteArray::append logic #8486
• feat(byte_array): add ByteArray::span and Span::len #8329
• refactor(test): remove fixed-size bytearray testutils #8343
• refactor(test): remove compare_spans util #8342
• refactor(test): remove compare_byte_array util and rework tests #8336
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[orizi]

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

---

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

            remainder_word: 0,
            remainder_len: downcast(0).unwrap(),
        }

Suggestion:

            data: [].span(),
            first_char_start_offset: 0,
            remainder_word: 0,
            remainder_len: 0,
        }

---

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

impl ByteSpanIntoByteArray of Into<ByteSpan, ByteArray> {
    fn into(mut self: ByteSpan) -> ByteArray {
        let start_offset = upcast(self.first_char_start_offset);

delay the upcast - as not really needed for comparison.

Code quote:

        let start_offset = upcast(self.first_char_start_offset);

---

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

        let start_offset = upcast(self.first_char_start_offset);
        // Span is aligned to word boundaries.
        if start_offset == 0 || self.data.is_empty() {

this sounds possibly wrong - as if you had the bytearray "short" and took the sub "hort" you would have empty data, but this implementation would be wrong.

Code quote:

        if start_offset == 0 || self.data.is_empty() {

---

corelib/src/test/byte_array_test.cairo line 555 at r1 (raw file):

fn test_span_into_bytearray() {
    let empty_ba: ByteArray = "";
    assert_eq!(empty_ba.span().into(), empty_ba, "empty round-trip");

remove at most tests - this does not add clarity here.

Suggestion:

    assert_eq!(empty_ba.span().into(), empty_ba);

[giladchase]

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

---

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

Previously, orizi wrote…

delay the upcast - as not really needed for comparison.

Done.

---

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

Previously, orizi wrote…

this sounds possibly wrong - as if you had the bytearray "short" and took the sub "hort" you would have empty data, but this implementation would be wrong.

I'm testing for this case in the next PR, see test_span_slice_under_31_bytes. It works as is, but the conditional (or the way ByteSpan::slice handles < 31 byte slices) is a bit misleading.

Slices that are < 31 bytes (so are held in pending/remainder word as a felt) split off the start prefix and save the word offset-free.

Meaning, the the structure of "short".span().slice(1,4) would be:

ByteSpan {
   data: [],
   remainder: "hort"
   start_offset: 0
}

rather than

ByteSpan {
   data: [],
   remainder: "short"
   start_offset: 1
}

Motivation for this is detailed in the slice PR, but the tldr is that the logic is simpler, and slice representation more consistent, if we add an invariant that the remainder word cannot have a start offset, just an end offset.
There is a small overhead for this of course (cost of split_bytes31), but i wanted to optimize for simplicity and consistency for starters.

Added a comment to clarify this.

---

corelib/src/test/byte_array_test.cairo line 555 at r1 (raw file):

Previously, orizi wrote…

remove at most tests - this does not add clarity here.

Done, tnx for letting me know these aren't required 🙏

---

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

            remainder_word: 0,
            remainder_len: downcast(0).unwrap(),
        }

🙏 Not sure why i thought this wasn't possible.

[orizi]

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

---

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

Previously, giladchase wrote…

I'm testing for this case in the next PR, see test_span_slice_under_31_bytes. It works as is, but the conditional (or the way ByteSpan::slice handles < 31 byte slices) is a bit misleading.

Slices that are < 31 bytes (so are held in pending/remainder word as a felt) split off the start prefix and save the word offset-free.

Meaning, the the structure of "short".span().slice(1,4) would be:

ByteSpan {
   data: [],
   remainder: "hort"
   start_offset: 0
}

rather than

ByteSpan {
   data: [],
   remainder: "short"
   start_offset: 1
}

Motivation for this is detailed in the slice PR, but the tldr is that the logic is simpler, and slice representation more consistent, if we add an invariant that the remainder word cannot have a start offset, just an end offset.
There is a small overhead for this of course (cost of split_bytes31), but i wanted to optimize for simplicity and consistency for starters.

Added a comment to clarify this.

i'm not sure it is better than allowing the slice itself to be simplest.
imagine, slice of slice of slice, and evaluation only here - this causes actual slicing of data with work, while when doing the final iteration should have been the only actual pricy action.

[giladchase]

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

---

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

Previously, orizi wrote…

i'm not sure it is better than allowing the slice itself to be simplest.
imagine, slice of slice of slice, and evaluation only here - this causes actual slicing of data with work, while when doing the final iteration should have been the only actual pricy action.

Done.

Discussed offline: start offset will be applied lazily only in into, end-offset is shifted-right on the spot (otherwise we have to save an extra field on ByteSpan in order to apply it lazily, as discussed offline).

[orizi]

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

---

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

    fn into(mut self: ByteSpan) -> ByteArray {
        let remainder_len = upcast(self.remainder_len);
        let Some(first_word) = self.data.pop_front() else {

can probably mix the code a bit the the rest as well.

Suggestion:

    fn into(mut self: ByteSpan) -> ByteArray {
        if self.first_char_start_offset == 0 {
            let mut ba = ByteArray { 
                data: self.data.into(),
                pending_word: 0,
                pending_word_len: 0,
            };
            ba.append_word(self.remainder_word, self.remainder_len);
            return ba;
        }
        let remainder_len = upcast(self.remainder_len);
        let Some(first_word) = self.data.pop_front() else {

[orizi]

@orizi reviewed 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase and @TomerStarkware)

[orizi]

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

[orizi]

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

[orizi]

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

[orizi]

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

[orizi]

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

[graphite-app]

Merge activity

• Oct 8, 9:26 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.