Hitting unreachable code in Chrono (and other direct callers of collect_str)
#32
Comments
|
Hey @caemor, any chance you could put together a minimal repro of this? I think the most important thing would be the struct definition + code necessary to trigger this (and maybe the version of Serde/derive you used). Once I have that, I'd be happy to look into implementing that, or at least turning it into a more meaningful error |
|
@jamesmunns I hit this with use chrono::{DateTime, Utc};
use serde::{Deserialize, Serialize};
#[derive(Serialize, Deserialize, Debug)]
pub struct Demo {
ts: DateTime<Utc>,
}
pub fn main() -> anyhow::Result<()> {
let dm = Demo { ts: Utc::now() };
dbg!(&dm);
let bytes = postcard::to_stdvec(&dm)?;
dbg!(&bytes);
Ok(())
} |
|
Awesome, thank you @bearcage for the reproduction case! I'll see if I can dive in and either improve the error, or see if there is a way to make it actually work. I'm open to PRs if anyone gets to this before me :) |
jamesmunns
changed the title
collect_str)
|
@jamesmunns I took a look at what this is meant to do this evening to see if I could come up with a reasonable implementation — tl;dr got a few ideas, but nothing that quite universally solves it. As I understand it, One potentially interesting way to come at this would be to add a For the heapless and slice flavors where we can't allocate, we could either error out with an "oops sorry your variable length string stuff needs an allocator," or possibly use the as-yet-unwritten portion of the slice or
In both cases it's a little fiddly because the max size for encoding I haven't messed with actually implementing any of those just yet, though, just musing on it while doing the dishes. |
|
Just wanted to add a +1 to this; I also just ran into this issue, which is sort of problematic, as I was sort of hoping to be able to add native Also, thanks so much for this crate! It's generally been pretty great to work with, and I'm very appreciative that it's available. |
|
This will be fixed in v1.0.0, I've figured out what causes this (serializing |
|
Hey @caemor and @bearcage, I've addressed this by making This isn't optimal, because we make two passes, but in most cases the total data formatted should be relatively short, and hopefully has a lower cost than using @bearcage's I'm open to improvements to this implementation, but hopefully this is enough for most usages for now! |
|
That'll do the job for me, thanks for coming back to this one! |
|
Thanks, thats enough for my usage! |
Serde supports serializing `Display` values via `collect_str`, without having to explicitly buffer them ahead of time. Using this for `Debug` values produced by `tracing` means the ktrace protocol implementation no longer needs to allocate. The downside is that postcard implements `collect_str` by processing the value twice, to determine its length. However, as jamesmunns/postcard#32 points out, this is likely fine since the data shouldn't be large.
When trying to replace bincode with postcard::to_stdvec I am reaching
unreachable-code here:postcard/src/ser/serializer.rs
Line 263 in 0d25b2c
I haven't looked further into it yet. I was just trying to do some benchmarks on size (and later speed) for my specific blobs.
Greetings,
Chris
Backtrace
The text was updated successfully, but these errors were encountered: