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

Use u32 to represent variant tags. #10

Merged
merged 1 commit into from Jan 5, 2015
Merged

Use u32 to represent variant tags. #10

merged 1 commit into from Jan 5, 2015

Conversation

@bitonic
Copy link
Collaborator

bitonic commented Jan 3, 2015

This makes it possible to exchange binary serialized structures between
different architectures, and reduces bandwidth. I doubt it's even
possible to have more than 4 billions variants anyway :).

This makes it possible to exchange binary serialized structures between
different architectures, and reduces bandwidth.  I doubt it's even
possible to have more than 4 billions variants anyway :).
@TyOverby
Copy link
Collaborator

TyOverby commented Jan 3, 2015

Right now my code works between architectures because self.read_uint() and self.emit_uint() are producing u64 on both x86 and x86-64. However, I do buy your argument that 32-bits are more than enough. I'll merge this when I figure out what is happening with the Travis-CI build.

@TyOverby
Copy link
Collaborator

TyOverby commented Jan 3, 2015

Actually, what about forcing emit_uint() to fit in a u32 instead of a u64? I'll need to bump the version number because it is a breaking change, but it will make it so that once a message is encoded somewhere, it can be decoded anywhere.

@bitonic
Copy link
Collaborator Author

bitonic commented Jan 3, 2015

I hadn't noticed that you use write_be_u64 for uint. I think that's a bad idea, I would use write_uint and leave the responsibility to the user -- code that works with machine dependent types are always going to have different semantics on different machines.

On the other hand, I think that forcing uint to u32 is an even worse idea, because it's unexpected and without any good reason.

The build might be failing because you're depending on an outdated rustc-serialize.

@TyOverby
Copy link
Collaborator

TyOverby commented Jan 3, 2015

The thing is that uint shows up a lot in rust data structures like Vec and HashMap, so it's not really their fault. I could do the checking on the deserialization side as an alternative. If it tries to deserialize a value that is to big to fit in a 32-bit uint, it could fail.

@TyOverby
Copy link
Collaborator

TyOverby commented Jan 3, 2015

Also, yeah, the build was failing because I was using an old version of rustc-serialize and because rustc broke derive(RustcSerialize) unless you have a flag enabled.

@bitonic
Copy link
Collaborator Author

bitonic commented Jan 4, 2015

Mhm, right, I guess serializing uint in a machine dependent way could be problematic if a lot of established data structures use it. In any case, I think falling back to u64 is a better choice, if we have to fall back to something. I just checked and the cereal and binary Haskell libraries do the same.

What's certain is that encoding variant tags to u32 is a win :).

TyOverby added a commit that referenced this pull request Jan 5, 2015
Use u32 to represent variant tags.
@TyOverby TyOverby merged commit 44f8536 into servo:master Jan 5, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@TyOverby
Copy link
Collaborator

TyOverby commented Jan 5, 2015

Thanks for the pull request, I've thought about it for a bit and decided to merge!

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

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