Implement serde support for StringPool
#12563
Conversation
| impl serde::ser::Serialize for Atom { | ||
| fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error> | ||
| where | ||
| S: serde::Serializer, | ||
| { | ||
| serde::ser::Serialize::serialize(&self.index, serializer) | ||
| } | ||
| } | ||
|
|
||
| impl<'de> serde::de::Deserialize<'de> for Atom { | ||
| fn deserialize<D>(deserializer: D) -> core::result::Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let index = serde::de::Deserialize::deserialize(deserializer)?; | ||
| Ok(Self { index }) | ||
| } | ||
| } |
There was a problem hiding this comment.
I've generally been avoiding derives to avoid the compile time hit -- not worth it?
There was a problem hiding this comment.
Oh no that seems pretty reasonable yeah. There are so few derive-compatible-impls in this crate I think it's reasonable to avoid for that reason
|
|
||
| while let Some(s) = seq.next_element::<String>()? { | ||
| debug_assert_eq!(s.len(), s.capacity()); | ||
| let s = s.into_boxed_str().expect("len == cap"); |
There was a problem hiding this comment.
Personally I'd say that this should use ? since while postcard might provide this guarantee a future deserializer might not, so in the spirit of being relatively flexible in core data structures I think it'd be good to propagate the error instead of assert here.
I also feel like it's conventionally a bit easier to reason about "always use ?" and sometimes there's a reserve for performance. Otherwise all .expect(...) needs to be double-checked against some reserve or some other condition nearby which can be a bit confusing
There was a problem hiding this comment.
while
postcardmight provide this guarantee a future deserializer might not
It is our own implementation of Deserialize for wasmtime_core::alloc::String that provides this guarantee. Does that change the calculus for you?
There was a problem hiding this comment.
I suppose a bit, but in general I still feel like I'd lean towards ?-and-forget. I don't think there's any reasonable way we can expect everyone to keep all the various constraints in their heads about reservation/capacity/etc and it's far easier to just use ? on methods. If the ? is dynamically dead that doesn't seem any worse to me than .expect("...") being dynamically dead
github-actions
bot
added
the
wasmtime:api
2 participants
Depends on #12562