-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat: add function for computing the postcard serialized size of a value. #86
Conversation
✅ Deploy Preview for cute-starship-2d9c9b canceled.
|
@jamesmunns I am wondering if the |
Hey @dignifiedquire, thanks for putting this together! I have some thoughts, let me know what you think: I think we could avoid all of the code duplication in Lines 134 to 209 in 7d07930
I think this will be more maintainable than remembering to keep the main serializer and the size serializer in sync, as flavors have a much simpler API, and I think cover what is needed here. This MAY not play nice with things like the If you're not sure how to do this, lemme know, and I can either help you out, or take this on at a later date. I don't think the current implementation would work with say, "to_slice" and "to_slice_cobs", because they will have different "on the wire" sizes. You really only can accurately determine the size with the "right" serializer flavor taken into account. Again - I think this should be handled by using a counting serializer, instead of the flavor being
So, I touch on this in #46 (comment), but there are really two things going on here:
The latter will have a larger runtime cost to handle, but using something like
I think at least most embedded folks will end up using |
The tricky piece for my usage here is that I would love to have the |
Yes very much, looks like this was pretty straightforward :) |
src/ser/flavors.rs
Outdated
@@ -410,3 +410,29 @@ where | |||
self.flav.finalize() | |||
} | |||
} | |||
|
|||
/// The `Size` flavor is a measurement flavor, storing the serialized bytes length into a plain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a missing thought here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
One minor comment, otherwise overall looks good! If you have time to add some doctest examples, it would be appreciated, but we can merge without if you're in a rush. |
will update tomorrow |
added a small doctest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks so much!
Closes #46.
Based on #66.