-
Notifications
You must be signed in to change notification settings - Fork 61
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 UnmarshalFirst #398
Add UnmarshalFirst #398
Conversation
5f79917
to
d833b05
Compare
A few rough spots with lint checks and accidentally using go1.20 features, but I think it's good to review now. |
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.
Thanks for contributing this PR! I really appreciate you explaining your use case and including benchmarks!
BTW, UnmarshalPrefix
can also be used for decoding CBOR Sequences (RFC 8742) without creating Decoder
! 👍
Some minor suggestions:
- Rename
UnmarshalPrefix
toUnmarshalFirst
. - Add
UnmarshalFirst
function inDecMode
interface. - A few minor tweaks to comments (e.g. used "CBOR data item").
d833b05
to
5b4dd49
Compare
Signed-off-by: Michael Andersen <[email protected]>
5b4dd49
to
d3a8ced
Compare
Thank you for your timely review! I have made the suggested changes, and rebased against master to incorporate the valid-to-wellformed change |
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.
LGTM! Thanks @immesys!
Description
In a similar vein to #397, there are some places in our code where we have a CBOR header in front of non-CBOR data. As we started integrating the decoder-based approach, we found there were places where we already had the data as a slice (not a reader), and the data items were sufficiently small that the overhead of constructing a reader over the slice so as to use the decoder became significant [1].
As a separate, but related issue, after we upgraded to 2.5.0, we found there were places in our code that relied upon the old behavior of Unmarshal being OK with the data slice having extraneous data, and it took us a little bit to find the issue as the comment on Unmarshal doesn't explicitly mention the new behavior.
This PR hopes to solve both of those issues by:
[1] This PR also adds two benchmarks that show what I mean about performance difference between Unmarshal and Decode for smaller items. An excerpt of that from my Macbook Pro M1
PR Was Proposed and Welcomed in Currently Open Issue
I skipped the "propose in issue" step again, but as before, please feel free to propose a completely different solution and I will change the code (or to reject the idea entirely). No hard feelings.
Checklist (for code PR only, ignore for docs PR)
Last line of each commit message should be in this format:
Signed-off-by: Firstname Lastname [email protected]
(see next section).
Certify the Developer's Certificate of Origin 1.1
the Developer Certificate of Origin 1.1.