Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds trait implementations #730

Merged
merged 4 commits into from
Mar 29, 2024
Merged

Adds trait implementations #730

merged 4 commits into from
Mar 29, 2024

Conversation

zslayton
Copy link
Contributor

Prior to this PR, the AnnotatableValueWriter trait provided many of the same methods as ValueWriter for ergonomics. Calls to these methods were delegated to the type returned from without_annotations().

This patch reifies the relationship between AnnotatableValueWriter and ValueWriter by moving the delegated methods into a blanket implementation of ValueWriter for all AnnotatableValueWriter implementations. Existing uses of these methods continue to work as expected, but instances of an AnnotatableValueWriter implementation can now also be passed into methods that expect an impl ValueWriter.

The PR also adds WriteAsIon implementations for a variety of Rust int types.

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

@zslayton zslayton changed the title Adds trait implemetnations Adds trait implementations Mar 28, 2024
@zslayton zslayton requested review from popematt and nirosys March 28, 2024 19:50
Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ PR tour

@@ -1494,7 +1494,7 @@ mod value_tests {
let list: Element = ion_list![1, 2, 3].into();
thread::scope(|_| {
// Move `list` into this scope, demonstrating `Send`
let elements = vec![list];
let elements = [list];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Clippy noticed that this vec! (and the associated heap allocation) is not necessary.

@@ -38,46 +38,6 @@ pub trait AnnotatableValueWriter: Sized {

/// Performs no operations and returns a [`ValueWriter`].
fn without_annotations(self) -> Self::ValueWriter;

// Users can call `ValueWriter` methods on the `AnnotatedValueWriter` directly. Doing so
// will implicitly call `without_annotations`.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ These default delegated methods mimicked those on ValueWriter, allowing users to call them on an AnnotatableValueWriter without first calling with_annotations or without_annotations. However, an AnnotatableValueWriter could not be passed to methods that required an implementation of ValueWriter.

delegate_value_writer_to!(closure |self_: Self| {
self_.without_annotations()
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ The new blanket implementation of ValueWriter for all AnnotatableValueWriters.

@@ -78,87 +78,3 @@ impl MakeValueWriter for Never {
}

impl MacroArgsWriter for Never {}

impl ValueWriter for Never {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Never implements many traits so it can be used as a marker to statically indicate unreachable/unused code paths. Previously, Never implemented both ValueWriter and AnnotatableValueWriter; the new blanket impl allows us to eliminate the explicit implementation of ValueWriter.

@zslayton zslayton merged commit 67e2520 into main Mar 29, 2024
28 checks passed
@zslayton zslayton deleted the more-trait-impls branch March 29, 2024 00:26
@zslayton zslayton restored the more-trait-impls branch March 29, 2024 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants