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

Implement Versionize for VecDeque #13

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexandruag
Copy link

@alexandruag alexandruag commented Jun 2, 2020

Reason for This PR

Added an implementation of Versionize for VecDeque to primitives.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

  • 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.

Signed-off-by: Alexandru Agache <[email protected]>
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

LGTM, just one small suggestion.

app_version: u16,
) -> VersionizeResult<Self> {
let mut v = VecDeque::new();
let len: u64 = bincode::deserialize_from(&mut reader)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit-ish:

Serialized len() is usize: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.len

Let's use the same thing here just in case u64 differs in size from usize:

Suggested change
let len: u64 = bincode::deserialize_from(&mut reader)
let len: usize = bincode::deserialize_from(&mut reader)

) -> VersionizeResult<()> {
// Serialize in the same fashion as bincode:
// Write len.
bincode::serialize_into(&mut writer, &self.len())
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 rely on the Vec primitive here and just use Into/From instead of duplicating the Vec code:

Docs say: This avoids reallocating where possible, but the conditions for that are strict, and subject to change, and so shouldn't be relied upon unless the Vec came from From<VecDeque> and hasn't been reallocated.

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