Skip to content

/ bincode

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

WIP Implement nocopy support for bincode #150

Merged
merged 6 commits into from Apr 21, 2017
Merged

WIP Implement nocopy support for bincode #150

merged 6 commits into from Apr 21, 2017
+193 −47

Conversation

@TyOverby
Copy link
Collaborator

TyOverby commented Apr 20, 2017 •
edited

TODO:

  • Find origina of test failure and fix it
  • Make deserialize_str and deserialize_bytes behave correctly in the face of a SizeLimit
  • Make BincodeRead public but prevent implementation
  • Stop using pub(crate)
TyOverby added 2 commits Apr 20, 2017
@TyOverby
basic infrastructure
e9d5301
@TyOverby
use BincodeRead internally
Loading status checks…
510fe5e
@TyOverby TyOverby requested a review from dtolnay Apr 20, 2017
@TyOverby TyOverby changed the title Implement nocopy support for bincode WIP Implement nocopy support for bincode Apr 20, 2017
@dtolnay
dtolnay reviewed Apr 20, 2017
View changes
Cargo.toml Outdated
@@ -14,8 +14,8 @@ description = "A binary serialization / deserialization strategy that uses Serde
[dependencies]
byteorder = "1.0.0"
num-traits = "0.1.32"
serde = { git = "https://github.com/serde-rs/serde", branch = "master" }
serde = "1.0.*"

This comment has been minimized.

Sign in to view
@dtolnay

dtolnay Apr 20, 2017

Collaborator

This disallows upgrading to 1.1.0 which is not a breaking change. I think this should say "1.0".
@TyOverby
fix zero copy behavior
Loading status checks…
dba9b98
@dtolnay
dtolnay reviewed Apr 20, 2017
View changes
src/de/mod.rs Outdated
@@ -8,6 +8,9 @@ use serde_crate::de::IntoDeserializer;
use serde_crate::de::Error as DeError;
use ::SizeLimit;
use super::{Result, Error, ErrorKind};
use self::read::BincodeRead;

pub(crate) mod read;

This comment has been minimized.

Sign in to view
@dtolnay

dtolnay Apr 20, 2017

Collaborator

pub(crate) doesn't work on stable yet so I would strongly discourage you from releasing with it.

This comment has been minimized.

Sign in to view
@TyOverby

TyOverby Apr 20, 2017

Author Collaborator

Ah yep, this was mainly added just to just so I wouldn't have to think about it when I was prototyping the new module structure. I'll add this to my list of TODOs at the PR description.
@dtolnay
Copy link
Collaborator

dtolnay commented Apr 20, 2017

Other than that, and the checklist, I think that does it. Not so much code after all!
@TyOverby
Copy link
Collaborator Author

TyOverby commented Apr 20, 2017

@dtolnay Yeah, I was pretty surprised at how easy it was! (I think you helped out a lot with your initial 1.0 change though)
@KodrAus KodrAus mentioned this pull request Apr 20, 2017
Closed
@ghost
ghost reviewed Apr 20, 2017
View changes
Cargo.toml
@@ -14,8 +14,8 @@ description = "A binary serialization / deserialization strategy that uses Serde
[dependencies]
byteorder = "1.0.0"
num-traits = "0.1.32"
serde = { git = "https://github.com/serde-rs/serde", branch = "master" }
serde = "1.*.*"

This comment has been minimized.

Sign in to view
@ghost

ghost Apr 20, 2017

I don't like this, you should just write "^1", or more simply "1".

This comment has been minimized.

Sign in to view
@TyOverby

TyOverby Apr 21, 2017

Author Collaborator

This is more explicit and easier to read.
TyOverby added 3 commits Apr 21, 2017
@TyOverby
rearrange module layout; remove pub(crate)
Loading status checks…
b68f1eb
@TyOverby
add size-limit safety checks when deserializing str or bytes
Loading status checks…
44c7fab
@TyOverby
fix-up tests
Loading status checks…
02c5cf7
@TyOverby
Copy link
Collaborator Author

TyOverby commented Apr 21, 2017

@dtolnay: I think this one is ready to merge in!
@TyOverby TyOverby merged commit 3b9387b into master Apr 21, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@dtolnay
dtolnay reviewed Apr 21, 2017
View changes
src/lib.rs
mod serde;
mod ser;
mod de;
pub mod internal;

This comment has been minimized.

Sign in to view
@dtolnay

dtolnay Apr 21, 2017

Collaborator

This is not supposed to be public right?

This comment has been minimized.

Sign in to view
@TyOverby

TyOverby Apr 21, 2017

Author Collaborator

I renamed endian_choice to internal and kept it public. I'm debating if I should switch back to the old name though.
@heartsucker
Copy link

heartsucker commented May 7, 2017

Can you guys push this to crates.io? I'd like to be able to use this in my libs and not depend on git commits.
@TyOverby
Copy link
Collaborator Author

TyOverby commented May 8, 2017

@heartsucker pretty sure this is already published as 0.8.0
@TyOverby TyOverby deleted the nocpy branch Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@TyOverby @dtolnay @heartsucker
You can’t perform that action at this time.