Initial impl of a LazyRawAnyReader #621
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #621 +/- ##
==========================================
- Coverage 81.11% 81.11% -0.01%
==========================================
Files 123 123
Lines 22570 22572 +2
Branches 22570 22572 +2
==========================================
+ Hits 18308 18309 +1
- Misses 2574 2575 +1
Partials 1688 1688
☔ View full report in Codecov by Sentry. |
src/lazy/any_encoding.rs
Outdated
| fn value(&self) -> LazyRawAnyValue<'data> { | ||
| match &self.encoding { | ||
| LazyRawFieldKind::Text_1_0(f) => f.value().into(), | ||
| LazyRawFieldKind::Binary_1_0(f) => f.value().into(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🗺️ There's a subtle API challenge here.
Prior to this PR, the value() trait method required that implementers return a &D::Value; in the case of AnyEncoding, that would mean returning a &LazyRawAnyValue.
Any time a method returns a &T (not &'static T), there MUST be a T living inside one of the method's parameters. In this case, there would need to be a LazyRawAnyValue living inside self, which there isn't. Instead, self contains the raw materials to construct a LazyRawAnyValue: either a LazyRawTextValue or a LazyRawBinaryValue. We can't return a &LazyRawAnyValue because it's illegal to return a reference to a stack-allocated variable created in this method's scope.
Fortunately, both LazyRawTextValue and LazyRawBinaryValue are eligible to implement Copy. Neither owns any heap-allocated resources. I changed the trait to return a copy of the value instead of a reference to one, resolving this issue. There are minor ripples of this change through the rest of this PR.
There was a problem hiding this comment.
Now that as_value is returning a copy of the value and not a reference, it should be called to_value. I've captured that in #626 and will address it when the merge queue is empty.
src/lazy/any_encoding.rs
Outdated
| #[derive(Debug, Clone)] | ||
| pub struct LazyRawAnyStruct<'data> { | ||
| encoding: LazyRawStructKind<'data>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub enum LazyRawStructKind<'data> { | ||
| Text_1_0(LazyRawTextStruct<'data>), | ||
| Binary_1_0(LazyRawBinaryStruct<'data>), | ||
| } |
There was a problem hiding this comment.
Are these LazyRawAny* structs exposed to users of the crate, or are they just pub in this module? If just pub in this module, is there any reason these can't be collapsed to remove the extra layer of indirection? (And similarly for the other struct/enum combos.)
| #[derive(Debug, Clone)] | |
| pub struct LazyRawAnyStruct<'data> { | |
| encoding: LazyRawStructKind<'data>, | |
| } | |
| #[derive(Debug, Clone)] | |
| pub enum LazyRawStructKind<'data> { | |
| Text_1_0(LazyRawTextStruct<'data>), | |
| Binary_1_0(LazyRawBinaryStruct<'data>), | |
| } | |
| #[derive(Debug, Clone)] | |
| pub struct LazyRawAnyStruct<'data> { | |
| Text_1_0(LazyRawTextStruct<'data>), | |
| Binary_1_0(LazyRawBinaryStruct<'data>), | |
| } |
There was a problem hiding this comment.
Offline discussion—the structs will eventually be public.
There was a problem hiding this comment.
The plan is for the structs to be pub, but the enums to be hidden. That way they don't have to be marked non_exhaustive and we can add other formats/versions over time.
Builds on outstanding PRs #609, #612, #613, #614, #616, #617, #619, and #620.
Adds the
LazyRawAnyReader, aLazyDecoderimplementation that can read either text or binary Ion.Because
LazyDecoderis a family of types, using it viaBox<dyn>would require boxing and dynamic dispatch at multiple levels. This implementation of theLazyRawAnyReaderavoids that problem by defining several new types that hold either a text or binary reader in anenumand delegates method calls to the one in use.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.