-
Notifications
You must be signed in to change notification settings - Fork 261
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
Conversation
src/serde/mod.rs
Outdated
@@ -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)); |
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.
BigEndian
here?
src/serde/mod.rs
Outdated
@@ -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); |
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.
deserialize_from
not generic over byte order?
@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. |
@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? |
6434853
to
db0b3d8
Compare
db0b3d8
to
dafeb04
Compare
@aldanor: I've gone with the decision to add an |
Oh, also, I've picked LittleEndian as the new default endianness (mainly for speed on the average computer) |
tests/test.rs
Outdated
let x = 10u64; | ||
let little = serialize::<_, byteorder::LittleEndian>(&x, Infinite).unwrap(); | ||
let big = serialize::<_, byteorder::BigEndian>(&x, Infinite).unwrap(); | ||
assert!(little != big); |
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.
Wouldn't it be better to use assert_ne!(little, big);
? As I understand it you get better error messages that way.
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.
huh, I didn't actually know about that one
eda2fb4
to
9df6a1e
Compare
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? |
While introducing selectable endianness in bincode-org#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.
…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.
No description provided.