-
Notifications
You must be signed in to change notification settings - Fork 21
VersionMap must contain all structs at their current versions #5
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
base: main
Are you sure you want to change the base?
Conversation
b4e199a to
73b8502
Compare
|
Changelog update required. |
767d0b3 to
9c4fa73
Compare
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 good. Some comments should be modified and also the commit message should be updated with the struct/union/enum observation (plus verion typo).
...all structs/enums/unions at their current versions. This commit also bumps the crate version to 0.1.2 Signed-off-by: Ioana Chirca <chioana@amazon.com>
9c4fa73 to
003a83d
Compare
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.
This looks good, but has a bit of downside in the sense it doesn't help with structures where we implement Versionize manually. Looks like doing the check in VersionMap::get_type_version itself covers more cases. That's not really possible right now because that method expects the type_id directly as a parameter, so we don't have access to the current version of the corresponding type. Would it make sense to change its signature to something like pub fn get_type_version<T: Versionize>(&self, root_version: u16) -> u16?
This also mitigates the binary size increase due to the current solution with generated code. |
Sounds better this way, thanks for pointing this out! |
Signed-off-by: Ioana Chirca chioana@amazon.com
If we are (de)serializing at the latest version from VersionMap, check that the struct's current version matches the version from the map. If it doesn't, it means that the Version Map is outdated.
Running
cargo benchinfirecracker/src/snapshotcrate after updating theversionize_derivedependency gives us these mean times:The extra checks do not introduce a regression (current baseline here)
Description of Changes
VersionizeErrorto be in scope, which is not necessarily true for the crates usingversionize_derive)License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).unsafecode is properly documented.CHANGELOG.md.