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

add serde serialization support #210

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

keithamus
Copy link
Contributor

This starts work on #63 by adding support for Serialize on Box and Vec, with some basic tests & docs.

I couldn't figure out the right way to implement DeserializeSeed for Vec. I'm a newcommer to rust so I think I need some pointers on how to handle that, as I'm struggling with lifetimes:

#[cfg(feature = "serde")]
impl<'a, T> DeserializeSeed<'a> for Vec<'a, T>
where
    T: Deserialize<'a>,
{
    type Value = ();

    fn deserialize<D>(mut self, deserializer: D) -> Result<Self::Value, D::Error>
    where
        D: Deserializer<'a>,
    {
        struct VecVisitor<'a, T: 'a>(&'a mut Vec<'a, T>);

        impl<'a, T> Visitor<'a> for VecVisitor<'a, T>
        where
            T: Deserialize<'a>,
        {
            type Value = ();

            fn expecting(
                &self,
                formatter: &mut core_alloc::fmt::Formatter,
            ) -> core_alloc::fmt::Result {
                Ok(())
            }

            fn visit_seq<V>(self, mut visitor: V) -> Result<(), V::Error>
            where
                V: SeqAccess<'a>,
            {
                while let Some(elem) = visitor.next_element()? {
                    self.0.push(elem);
                }
                Ok(())
            }
        }

        deserializer.deserialize_seq(VecVisitor::<'a>(self.as_mut()))
    }
}

Copy link
Owner

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks! And sorry for the delay giving feedback on this. A handful of comments below.

README.md Outdated Show resolved Hide resolved
src/boxed.rs Outdated Show resolved Hide resolved
src/boxed.rs Outdated Show resolved Hide resolved
src/collections/vec.rs Outdated Show resolved Hide resolved
Comment on lines 20 to 23
let boxed = Box::new_in(1, &bump);
assert_eq!(serde_json::to_string(&boxed).unwrap(), "1");
let boxedStr = Box::new_in("a", &bump);
assert_eq!(serde_json::to_string(&boxedStr).unwrap(), "\"a\"");
Copy link
Owner

Choose a reason for hiding this comment

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

Can we also test a few other things:

  • tuple
  • struct w/ a couple fields
  • enum w/ a couple variants
  • vec w/ a couple elements
  • hash map
  • hash set

It should be fine to add a dev dependency on serde_derive if it makes this easier, fwiw.

Also, no need to explicitly write out the expected JSON; should be able to call serde_json::to_string(&unboxed_value) and then serde_json::to_string(&boxed_value) and compare the results for equality, I believe. Or compare std::boxed::Box<T> with bumpalo::boxed::Box<'_, T>. Might be worth writing a little helper that does this dance, rather than writing it out in full for all the cases above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a struct with an enum and option. Hopefully that's sufficient, or would you like me to add more such as the hash map/set?

tests/all/vec.rs Outdated Show resolved Hide resolved
@keithamus keithamus force-pushed the add-serde-serialization-support branch from 868c118 to 3596757 Compare April 6, 2024 08:47
@keithamus keithamus requested a review from fitzgen April 6, 2024 08:50
Copy link
Owner

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks!

@fitzgen fitzgen merged commit d746a56 into fitzgen:main Apr 8, 2024
8 checks passed
@keithamus keithamus deleted the add-serde-serialization-support branch April 8, 2024 22:21
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