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

Make Reader and Writer generic on Endianness #103

Merged
merged 4 commits into from Feb 25, 2017
Merged

Make Reader and Writer generic on Endianness #103

merged 4 commits into from Feb 25, 2017

Conversation

@TyOverby
Copy link
Collaborator

TyOverby commented Feb 1, 2017

No description provided.

@@ -69,7 +70,7 @@ pub fn serialize<T>(value: &T, size_limit: SizeLimit) -> SerializeResult<Vec<u8>
SizeLimit::Infinite => Vec::new()
};

try!(serialize_into(&mut writer, value, SizeLimit::Infinite));
try!(serialize_into::<_, _, ::byteorder::BigEndian>(&mut writer, value, SizeLimit::Infinite));

This comment has been minimized.

@aldanor

aldanor Feb 3, 2017

BigEndian here?

@@ -107,7 +108,7 @@ pub fn deserialize_from<R, T>(reader: &mut R, size_limit: SizeLimit) -> Deserial
where R: Read,
T: serde::Deserialize,
{
let mut deserializer = Deserializer::new(reader, size_limit);
let mut deserializer = Deserializer::<_, BigEndian>::new(reader, size_limit);

This comment has been minimized.

@aldanor

aldanor Feb 3, 2017

deserialize_from not generic over byte order?

@TyOverby
Copy link
Collaborator Author

TyOverby commented Feb 3, 2017

@aldanor: in its current state, this PR adds endian genericness to the backend, but not the public functions. That will change in the future, but I wanted to get the difficult part passing tests first.

@aldanor
Copy link

aldanor commented Feb 5, 2017

@TyOverby I see then, thanks; looks good to me given that this will be later exposed to the crate-level functions.

// Should there be some explicit tests for big/little endian?

@ZoeyR ZoeyR mentioned this pull request Feb 9, 2017
@TyOverby TyOverby force-pushed the endian-choice branch from 6434853 to db0b3d8 Feb 10, 2017
@TyOverby TyOverby force-pushed the endian-choice branch from db0b3d8 to dafeb04 Feb 10, 2017
@TyOverby
Copy link
Collaborator Author

TyOverby commented Feb 10, 2017

@aldanor: I've gone with the decision to add an endian_choice module with endianness exposed.

@TyOverby
Copy link
Collaborator Author

TyOverby commented Feb 10, 2017

Oh, also, I've picked LittleEndian as the new default endianness (mainly for speed on the average computer)

let x = 10u64;
let little = serialize::<_, byteorder::LittleEndian>(&x, Infinite).unwrap();
let big = serialize::<_, byteorder::BigEndian>(&x, Infinite).unwrap();
assert!(little != big);

This comment has been minimized.

@ZoeyR

ZoeyR Feb 10, 2017

Collaborator

Wouldn't it be better to use assert_ne!(little, big);? As I understand it you get better error messages that way.

This comment has been minimized.

@TyOverby

TyOverby Feb 10, 2017

Author Collaborator

huh, I didn't actually know about that one

@TyOverby TyOverby force-pushed the endian-choice branch from eda2fb4 to 9df6a1e Feb 10, 2017
@aldanor
Copy link

aldanor commented Feb 11, 2017

That's one way to go about it I guess. Little endian as a default sounds grand; worth mentioning in the docstrings of the root module somewhere?

@TyOverby TyOverby merged commit 5cdf2a2 into master Feb 25, 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
antrik added a commit to antrik/bincode that referenced this pull request Mar 1, 2017
While introducing selectable endianness in
servo#103 , the new type parameter
has been hidden from the public `serialize()`, `deserialize()` etc.
functions, and only made available through an alternate API entry point.
The same kind of encapsulation also needs to be performed for the public
`Serializer` and `Deserializer` types.
TyOverby added a commit that referenced this pull request Mar 2, 2017
…128)

While introducing selectable endianness in
#103 , the new type parameter
has been hidden from the public `serialize()`, `deserialize()` etc.
functions, and only made available through an alternate API entry point.
The same kind of encapsulation also needs to be performed for the public
`Serializer` and `Deserializer` types.
@TyOverby TyOverby deleted the endian-choice branch Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.