Making DateTime data zero-copy #1550
Conversation
robertbastian
requested review from
gregtatum,
nordzilla,
sffc,
zbraniecki and
a team
as code owners
|
I did, which is why |
|
the zeromaps are a little bit less efficient than litemaps, serialization-wise, but i'm surprised it adds up to that much |
| @@ -56,24 +59,28 @@ macro_rules! symbols { | |||
| ($name: ident { $element: ident: Option<$ty: ty>, $($tokens: tt)+ } -> ($($members:tt)*)) => { | |||
| symbols!($name { $($tokens)* } -> ( | |||
| $($members)* | |||
| #[cfg_attr(feature = "provider_serde", serde(borrow))] | |||
| pub $element: Option<$ty>, | |||
There was a problem hiding this comment.
Issue: serde(borrow) does not always work through options. You may need deserialize_with.
This is a known issue that dtolnay does not intend to fix (see serde-rs/serde#2016)
There was a problem hiding this comment.
Please add tests to verify that the data being deserialized is actually being borrowed. #[serde(borrow)] is weird and it sometimes works and sometimes doesn't work.
There was a problem hiding this comment.
Ugh i was looking for Option but didn't notice this one since it's in a macro
There was a problem hiding this comment.
We can probably get a clippy lint that warns about serde(borrow) on option fields
|
I would also like to know where the 81 KB comes from. This is a destabilizing change and it should not have been merged when 0.5 is days-to-hours away. |
This reverts commit 405b9fd.
* Fixing Option borrows * Custom Option<Cow<str>> deserializer * Revert "Custom Option<Cow<str>> deserializer" This reverts commit a69941c. * Serde utils in icu_provider * Removing serde_with dep * fmt and fix * Update provider/core/src/serde/borrow_de_utils.rs Co-authored-by: Shane F. Carr <shane@unicode.org> Co-authored-by: Shane F. Carr <shane@unicode.org>
No description provided.