Adds binary read support for delimited arg groups #870
Conversation
| let bytes_to_read = match group_header_flex_uint.value() { | ||
| 0 => todo!("delimited argument groups"), | ||
| n_bytes => n_bytes as usize, | ||
| }; | ||
| // If it's length-prefixed, we don't need to inspect its contents. We can build an | ||
| // ArgGroup using the unexamined bytes; we'll parse them later if they get evaluated. | ||
| let arg_group_length = group_header_flex_uint.size_in_bytes() + bytes_to_read; | ||
| let arg_group = BinaryEExpArgGroup::new( | ||
| parameter, | ||
| self.remaining_args_buffer.slice(0, arg_group_length), | ||
| group_header_flex_uint.size_in_bytes() as u8, | ||
| ); | ||
| ( | ||
| EExpArg::new(parameter, EExpArgExpr::ArgGroup(arg_group)), | ||
| self.remaining_args_buffer.consume(arg_group_length), | ||
| ) |
There was a problem hiding this comment.
🗺️ This segment was moved into the match arm on line 367, starting with n_bytes =>.
| match group_header_flex_uint.value() { | ||
| // A FlexUInt of `0` indicates that the arg group is delimited. | ||
| // We need to step through it to determine its span. | ||
| 0 if *parameter.encoding() == ParameterEncoding::Tagged => { |
There was a problem hiding this comment.
🗺️ This arm contains the new logic.
| n_bytes => { | ||
| let bytes_to_read = n_bytes as usize; | ||
| let arg_group_length = | ||
| group_header_flex_uint.size_in_bytes() + bytes_to_read; | ||
| let arg_group = BinaryEExpArgGroup::new( | ||
| parameter, | ||
| self.remaining_args_buffer.slice(0, arg_group_length), | ||
| group_header_flex_uint.size_in_bytes() as u8, | ||
| ); | ||
| ( | ||
| EExpArg::new(parameter, EExpArgExpr::ArgGroup(arg_group)), | ||
| self.remaining_args_buffer.consume(arg_group_length), | ||
| ) |
There was a problem hiding this comment.
🗺️ Relocated here from above.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #870 +/- ##
==========================================
+ Coverage 77.87% 77.91% +0.03%
==========================================
Files 136 136
Lines 34314 34385 +71
Branches 34314 34385 +71
==========================================
+ Hits 26723 26791 +68
- Misses 5630 5631 +1
- Partials 1961 1963 +2 ☔ View full report in Codecov by Sentry. |
| /// The iterator is parsing expressions from a slice of the input buffer | ||
| Input(BinaryBuffer<'top>), | ||
| /// The iterator is iterating over the bump-allocated cache of previously parsed expressions. | ||
| Cache(&'top [LazyRawValueExpr<'top, BinaryEncoding_1_1>]), |
There was a problem hiding this comment.
So, the name and doc comments suggest to me that these expressions could be shared somehow. (Maybe there's a variable that's used more than once...?) Yet, further down, I see that this is being mutated. Is this actually a slice that points to a cache that is owned somewhere else? And it's effectively just the start and length of the slice that is updated?
Are any of those cached expressions stateful? Are we sure that there's no possibility of interference between different use-sites of the same cached expressions?
There was a problem hiding this comment.
Both of BinaryEExpArgGroupIteratorSource's variants refer to data stored elsewhere with a lifetime of 'top:
- In the
Inputcase, theBinaryBufferholds a slice of the input buffer, which is frozen for the duration of'top. - In the
Cachecase, the reader bump-allocated a&'top [...]in which to store raw lexed expressions it found as it traversed a delimited group. Data stored in the bump is guaranteed to outlive'top.
BinaryEExpArgGroupIteratorSource instances are immutable, but they are each wrapped by a BinaryEExpArgGroupIterator that is mutable. Each time the iterator's next() method is called, it either advances the slice of the source's Input variant that it holds or it pops the head off of the slice of the Cache variant that it holds.
Both the iterator and its source are trivially copy-able, so you could easily/safely make several iterators traversing the expression group at once. Each iterator would mutate its own state as it traversed the immutable source data.
Is that helpful?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.