Binary 1.0 impl for new writer API #686

zslayton · 2023-12-08T15:19:00Z

Provides a binary 1.0 implementation of the LazyEncoder family of traits. The second commit is entirely refactoring moves, so reviewers should focus on the first commit's diff. A future PR will add a managed writer wrapper and ion-tests integration.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

zslayton

🗺️ PR tour

zslayton · 2023-12-08T15:28:39Z

src/lazy/encoder/annotate.rs


 /// Associates a value to serialize with a sequence of annotations.
 pub struct Annotated<'a, T: ?Sized, A> {
    value: &'a T,
    annotations: &'a [A],
 }

-/// Provides implementors with an extension method ([`annotate`](Annotate::annotate)) that allows
+/// Provides implementors with an extension method ([`annotate`](Annotate::annotated_with)) that allows


🗺️ Renamed annotate to annotated_with to address #681.

zslayton · 2023-12-08T15:29:25Z

Cargo.toml

@@ -71,7 +71,7 @@ num-integer = "0.1.44"
 num-traits = "0.2"
 arrayvec = "0.7"
 smallvec = {version ="1.9.0", features = ["const_generics"]}
-bumpalo = {version = "3.13.0", features = ["collections"]}
+bumpalo = {version = "3.14.0", features = ["collections", "std"]}


🗺️ The std feature supplies an implementation of std::io::Write for bumpalo's Vec type. (Aliased to BumpVec in this codebase.)

zslayton · 2023-12-08T15:31:31Z

src/lazy/encoder/binary/mod.rs

+    fn ptr_to_mut_ref<'a, T>(ptr: *mut ()) -> &'a mut T {
+        let typed_ptr: *mut T = ptr.cast();
+        unsafe { &mut *typed_ptr }
+    }


🗺️ I've marked this function unsafe in a commit I'll push shortly.

zslayton · 2023-12-08T15:33:47Z

src/lazy/encoder/binary/mod.rs

+    }
+}
+
+impl<W: Write> Sealed for LazyRawBinaryWriter_1_0<W> {}


🗺️ The dependency on the Sealed trait limits implementations of LazyRawWriter to types within the crate. We can relax this later as needed.

zslayton · 2023-12-08T15:34:19Z

src/lazy/encoder/binary/mod.rs

+    delegate! {
+        to self {
+            fn value_writer(&mut self) -> Self::ValueWriter<'_>;
+        }


🗺️ This use delegate! doesn't save much code, but it does convey the intent nicely.

zslayton · 2023-12-08T15:36:53Z

src/lazy/encoder/binary/value_writer.rs

+        self.encoding_buffer.as_slice()
+    }
+
+    pub fn write_symbol_id(mut self, symbol_id: SymbolId) -> IonResult<()> {


🗺️ All of the scalar encoding logic was copy/pasted from the old raw binary writer.

zslayton · 2023-12-08T15:38:28Z

src/lazy/encoder/binary/value_writer.rs

+    delegate! {
+        to self {


🗺️ Having the methods live on the type (not the trait) and then having the trait methods delegate to the inherent methods means that you can use the type itself without needing to import the trait.

zslayton · 2023-12-08T16:07:13Z

src/lazy/encoder/mod.rs

@@ -1,17 +1,17 @@
 #![allow(non_camel_case_types)]

-pub mod annotate;


🗺️ All of the deletions in this file were actually just moves to another module.

zslayton · 2023-12-08T16:31:52Z

src/lazy/encoder/value_writer.rs

+        &mut self,
+        name: A,
+        value: V,
+    ) -> IonResult<&mut Self>;


🗺️ This trait needs to also offer write_null(name, value), write_bool(name, value), etc. Tracking this in #687.

zslayton · 2023-12-08T16:33:30Z

src/lazy/encoder/write_as_ion.rs

 pub trait WriteAsIon {
-    fn write_as_ion<'a, W: Write + 'a, E: LazyEncoder<W>, V: AnnotatedValueWriter<'a, W, E>>(


🗺️ The LazyEncoder trait got simplified a lot in this PR. It only has a single associated type now: Writer<W>. As a result, a lot of these generics and lifetimes could be removed.

popematt · 2023-12-13T22:44:27Z

src/lazy/encoder/binary/value_writer.rs

+// The largest possible 'L' (length) value that can be written directly in a type descriptor byte.
+// Larger length values will need to be written as a VarUInt following the type descriptor.


minor nitpick—could this be a doc comment (so that it shows up in IDEs)?

src/lazy/encoder/binary/value_writer.rs

src/lazy/encoder/value_writer.rs

popematt · 2023-12-13T23:46:00Z

src/lazy/encoder/value_writer.rs

+    // XXX: For now, it's not possible to offer versions of `write_list`, `write_sexp`, or
+    //      `write_struct`. This is due to a point-in-time limitation in the borrow checker[1].
+    //      It is still possible to call (e.g.)
+    //          self.value_writer().list_writer(...)
+    //      as a workaround.
+    // [1]: https://blog.rust-lang.org/2022/10/28/gats-stabilization.html#implied-static-requirement-from-higher-ranked-trait-bounds


So, I had an idea since this note isn't very discoverable by users. We could add a no-op implementation of write_list, etc. that has an empty type as one of the function arguments. It's not callable because you can't create an argument to pass to it, but it can hold this note directing users to the workaround. Then, once this limitation is fixed, we can change the argument type of the function to the correct argument type.

I opened #691 to track this.

src/lazy/encoder/write_as_ion.rs

popematt · 2023-12-14T00:01:06Z

src/lazy/encoder/write_as_ion.rs

+impl_write_as_ion_value_for_iterable!(&[T], T);
+impl_write_as_ion_value_for_iterable!([T; N], T, const N: usize);
+
+pub trait WriteAsSExp<T>: Sized


I assume this is needed so that you can indicate that an iterable of some sort should be written as a sexp instead of a list (which I assume is the default).

popematt · 2023-12-14T00:04:25Z

So, I started by reviewing just the first commit, and now that I'm reading the next commits, I see you have already addressed some of the comments I left.

src/lazy/encoder/write_as_ion.rs

popematt

Approving because feedback will be addressed in a follow up PR.

…#692)

zslayton and others added 4 commits December 8, 2023 10:17

Adds binary Ion 1.0 impl of the new writer API

3d893d0

Moved code into more cohesive modules

93c824a

DRY via macros, some cleanup

9cce623

clippy suggestions

aafd9b2

zslayton commented Dec 8, 2023

View reviewed changes

zslayton marked this pull request as ready for review December 8, 2023 16:35

zslayton requested review from tgregg and popematt December 8, 2023 16:36

Added impl_write_as_ion_value! macro

0d0cabc

zslayton mentioned this pull request Dec 13, 2023

Skeleton impl of binary 1.1 writer #688

Merged

popematt reviewed Dec 14, 2023

View reviewed changes

src/lazy/encoder/write_as_ion.rs Show resolved Hide resolved

popematt approved these changes Dec 19, 2023

View reviewed changes

removed old test code

d150ee1

zslayton merged commit 0d16a73 into main Dec 20, 2023
20 checks passed

zslayton deleted the new-binary-writer branch December 20, 2023 12:43

zslayton mentioned this pull request Dec 20, 2023

Make discoverable placeholder container writer methods #691

Open

zslayton added a commit that referenced this pull request Dec 21, 2023

Incorporates feedback from PR #686

07d168f

zslayton added a commit that referenced this pull request Jan 9, 2024

Incorporates feedback from PR #686 (#692)

436ac0a

zslayton added a commit that referenced this pull request Jan 10, 2024

Adds reading/writing FlexInt, FlexUInt (#690), feedback from #686 (…

c109160

…#692)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binary 1.0 impl for new writer API #686

Binary 1.0 impl for new writer API #686

zslayton commented Dec 8, 2023 •

edited

Loading

zslayton left a comment

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

zslayton Dec 8, 2023

popematt Dec 13, 2023

popematt Dec 13, 2023

zslayton Dec 15, 2023

zslayton Dec 20, 2023

popematt Dec 14, 2023

popematt commented Dec 14, 2023

popematt left a comment

		@@ -1,17 +1,17 @@
		#![allow(non_camel_case_types)]

		pub mod annotate;

		pub trait WriteAsIon {
		fn write_as_ion<'a, W: Write + 'a, E: LazyEncoder<W>, V: AnnotatedValueWriter<'a, W, E>>(

		// The largest possible 'L' (length) value that can be written directly in a type descriptor byte.
		// Larger length values will need to be written as a VarUInt following the type descriptor.

Binary 1.0 impl for new writer API #686

Binary 1.0 impl for new writer API #686

Conversation

zslayton commented Dec 8, 2023 • edited Loading

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt commented Dec 14, 2023

popematt left a comment

Choose a reason for hiding this comment

zslayton commented Dec 8, 2023 •

edited

Loading