Skip to content
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 versionize for VecDequeue/HashMap/HashSet. #25

Closed

Conversation

imaginezz
Copy link

Signed-off-by: JasonBian[email protected]

Reason for This PR

Added support for some of the commonly used data structures: VecDeque, HashMap, and HashSet

Description of Changes

  • Add versionize support for new types in primitives.rs.
  • Add Error arms for new types in lib.rs.

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]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any user-facing changes are mentioned in CHANGELOG.md.

- add versionize support for VecDequeue/HashMap/HashSet.

Signed-off-by: JasonBian <[email protected]>
@imaginezz
Copy link
Author

#24

Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good overall. I have left some comments on the code.

Also please cover the new primitives in these tests: test_bincode_deserialize_from_versionize() and test_bincode_serialize_to_versionize()

@@ -333,6 +340,168 @@ where
}
}

impl<T> Versionize for VecDeque<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost like implementation for Vec. Can we do this with a macro to avoid duplicating code ?

.map_err(|ref err| VersionizeError::Serialize(format!("{:?}", err)))?;
// Walk the hash map and write each element.
for (k, v) in self.iter() {
(k.clone(), v.clone()).serialize(writer, version_map, app_version)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid the temporary clones here ?

)
.unwrap();

assert_eq!(store, restore);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add negative tests.

<HashSet<String> as Versionize>::deserialize(&mut snapshot_mem.as_slice(), &vm, 1)
.unwrap();

assert_eq!(store, restore);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@berciuliviu
Copy link

Hello @imaginezz ! Do you plan to continue working on this PR?

@imaginezz
Copy link
Author

I will close this pr, and update in #37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants