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 endianness to serializers and deserializers #155

Closed
wants to merge 1 commit into from

Conversation

@dlrobertson
Copy link
Collaborator

dlrobertson commented Feb 26, 2017

Enforce endianness in the creation of our serializers and deserializers
in the ipc module.

Resolves: #154

@antrik
Copy link
Contributor

antrik commented Feb 27, 2017

I'm not sure I understand. Is this working around a regression introduced by a change in the compiler? If so, is it really a good idea to work around it, rather than waiting for a fix in the compiler?...

src/ipc.rs Outdated
@@ -152,7 +153,7 @@ impl<T> IpcSender<T> where T: Serialize {
let os_ipc_shared_memory_regions;
let os_ipc_channels;
{
let mut serializer = bincode::Serializer::new(&mut bytes);
let mut serializer = bincode::Serializer::<_, LittleEndian>::new(&mut bytes);

This comment has been minimized.

@antrik

antrik Feb 27, 2017

Contributor

Shouldn't that rather be NativeEndian?

(Which is apparently hard-aliased to LittleEndian anyway, for whatever reason -- but still it seems a more obvious choice conceptually?...)

This comment has been minimized.

@dlrobertson

dlrobertson Feb 27, 2017

Author Collaborator

Ah, right... NativeEndian is much better.

@jdm
Copy link
Member

jdm commented Feb 27, 2017

This is the bincode crate publishing servo/bincode@5cdf2a2 as a non-semver-compatible version update, presumably (servo/bincode@eebd6a9).

@antrik
Copy link
Contributor

antrik commented Feb 27, 2017

Actually, if using NativeEndian -- and if that was indeed what the name suggests -- this would in fact be a good change regardless, since there is totally no reason to enforce any particular endianness here, and using the native one could be a tick more efficient on some machines. Otherwise... I'm not sure. Forcing little endian is probably still good, since it's the better choice in most cases, and it might not be default.

@antrik
Copy link
Contributor

antrik commented Feb 27, 2017

@jdm well, I don't quite understand the interaction between serde and bincode; but if it's indeed a bincode issue (and not compiler-related, as #154 suggests), I would assume it's caused by servo/bincode#103 -- but that was supposed to be API-neutral AIUI, in which case the regression should be fixed in bincode I guess?...

@antrik
Copy link
Contributor

antrik commented Feb 27, 2017

(For the record, even if unintentional, strictly speaking this is actually semver-compatible, since semver explicitly states that there are no API guarantees whatsoever for pre-releases... But then again, the Cargo interpretation of semver doesn't match the actual standard anyway.)

@dlrobertson dlrobertson force-pushed the dlrobertson:add_endianness branch from 0c695eb to ddf71bc Feb 27, 2017
@dlrobertson
Copy link
Collaborator Author

dlrobertson commented Feb 27, 2017

I don't quite understand the interaction between serde and bincode; but if it's indeed a bincode issue (and not compiler-related, as #154 suggests)

It is indeed a bincode/serde issue. I did not describe the issue well. I also do not know bincode/serde well.

Actually, if using NativeEndian -- and if that was indeed what the name suggests -- this would in fact be a good change regardless

Agreed.

This is the bincode crate publishing servo/bincode@5cdf2a2

Right. The addition of the endianness generics seems to be the cause.

Ensure that the endianness used is the native endianness of the system.
@dlrobertson dlrobertson force-pushed the dlrobertson:add_endianness branch from ddf71bc to 67c731d Feb 27, 2017
@antrik
Copy link
Contributor

antrik commented Mar 1, 2017

Actually, this workaround will work only with the breaking API change; so the patch at least has to update the bincode version requirement in Cargo.toml... However, when the regression is (hopefully) fixed upstream, it won't work with later versions either. (And the broken version presumably gets yanked, so in fact the workaround won't work with any version once that happens...)

In other words, this can probably be only addressed in a sane way in bincode itself -- which is really the right thing to do anyway. In the free software world, we generally can, and should, fix problems at their source, rather then working around them...

@antrik
Copy link
Contributor

antrik commented Mar 1, 2017

@dlrobertson dlrobertson closed this Mar 2, 2017
@dlrobertson dlrobertson deleted the dlrobertson:add_endianness branch Mar 2, 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

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