Adds binary read support for system eexps #869
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #869 +/- ##
==========================================
+ Coverage 77.86% 77.87% +0.01%
==========================================
Files 136 136
Lines 34273 34314 +41
Branches 34273 34314 +41
==========================================
+ Hits 26685 26723 +38
+ Misses 5631 5630 -1
- Partials 1957 1961 +4 ☔ View full report in Codecov by Sentry. |
| @@ -141,7 +141,7 @@ impl<'top> RawEExpression<'top, v1_1::Binary> for &'top BinaryEExpression_1_1<'t | |||
| type ArgGroup = BinaryEExpArgGroup<'top>; | |||
|
|
|||
| fn id(self) -> MacroIdRef<'top> { | |||
| MacroIdRef::LocalAddress(self.macro_ref.address()) | |||
| self.macro_ref.id() | |||
There was a problem hiding this comment.
🗺️ Throughout this patch, places using a "macro address" have been generalized to work with a "macro ID," allowing them to distinguish between an address in the application's macro table and one in the system table.
| let (macro_id, input_after_address) = match opcode.opcode_type { | ||
| EExpressionWith6BitAddress => ( | ||
| MacroIdRef::LocalAddress(opcode.byte as usize), | ||
| self.consume(1), | ||
| ), |
There was a problem hiding this comment.
🗺️ Each arm in this match now returns a MacroIdRef instead of a MacroAddress (usize).
| } | ||
| // Length-prefixed is a special case. | ||
| EExpressionWithLengthPrefix => return self.read_eexp_with_length_prefix(opcode), | ||
| SystemEExpression => { |
There was a problem hiding this comment.
🗺️ The actual read logic for binary e-expressions. This is the only arm that returns something other than a MacroIdRef::LocalAddress—it returns a MacroIdRef::SystemAddress.
| @@ -37,7 +37,7 @@ pub enum OpcodeType { | |||
| TypedNull, // 0xEB - | |||
| Nop, // 0xEC-0xED - | |||
| SystemSymbolAddress, // 0xEE | |||
| SystemMacroEExpression, // 0xEF - | |||
| SystemEExpression, // 0xEF - | |||
There was a problem hiding this comment.
🗺️ Renamed for consistency. *MacroEExpression was redundant anyway.
| assert_eq_expected(&actual, expected) | ||
| } | ||
|
|
||
| fn bin_stream_eq(input: &[u8], expected: &str) -> IonResult<()> { |
There was a problem hiding this comment.
🗺️ The unit test helper stream_eq was hardcoded to expect 1.1 text so each call site didn't have to repeat the 1.1 IVM. For this PR I needed to test reading binary streams, so I added an analogous bin_stream_eq.
There was a problem hiding this comment.
The diff generated here is not ideal, and that made me wonder which diffing algorithm GitHub uses. Apparently it (like other review tools I'm familiar with) uses the Git default (Myers): https://stackoverflow.com/questions/61769668/whats-the-diff-algorithm-used-by-github
The Patience algorithm would do better here. Here's an analogous example: https://gist.github.com/roryokane/6f9061d3a60c1ba41237
Nothing to do here, I'm just pointing it out so that from now on you can also be slightly dissatisfied when you encounter odd diffs.
EDIT: One thing you can do is +1 this community discussion? https://github.com/orgs/community/discussions/16782
| pub fn id_text(&'top self) -> Cow<'top, str> { | ||
| self.name() | ||
| .map(Cow::from) | ||
| .unwrap_or_else(move || Cow::from(format!("{}", self.address()))) | ||
| } |
There was a problem hiding this comment.
🗺️ This was unused.
| @@ -125,13 +124,16 @@ pub enum MacroKind { | |||
|
|
|||
| #[derive(Debug, Copy, Clone, PartialEq)] | |||
| pub struct MacroRef<'top> { | |||
| address: MacroAddress, | |||
| id: MacroIdRef<'top>, | |||
There was a problem hiding this comment.
🗺️ This allows us to track the ID used to look up the associated &'top Macro.
| assert_eq_expected(&actual, expected) | ||
| } | ||
|
|
||
| fn bin_stream_eq(input: &[u8], expected: &str) -> IonResult<()> { |
There was a problem hiding this comment.
The diff generated here is not ideal, and that made me wonder which diffing algorithm GitHub uses. Apparently it (like other review tools I'm familiar with) uses the Git default (Myers): https://stackoverflow.com/questions/61769668/whats-the-diff-algorithm-used-by-github
The Patience algorithm would do better here. Here's an analogous example: https://gist.github.com/roryokane/6f9061d3a60c1ba41237
Nothing to do here, I'm just pointing it out so that from now on you can also be slightly dissatisfied when you encounter odd diffs.
EDIT: One thing you can do is +1 this community discussion? https://github.com/orgs/community/discussions/16782
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.