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

Varint enum tags and lengths behind a feature #271

Closed
wants to merge 4 commits into from

Conversation

@maciejhirsz
Copy link
Contributor

maciejhirsz commented Jun 8, 2019

Resolves #157, resolves #75.

Varint support needs to be enabled with the varint feature, it only affects enum tags and sequence lengths, all fixed size ints travel over the wire without alterations.

The particular encoding is pretty straight forward: while n is greater then 127 keep taking lowest 7 bits and serialize them as u8 with significant bit set to 1, final byte is always encoded without significant bit. Varint ignores the endianess setting, partly because I think it's unnecessary in this case, partly because I didn't want to wrap my head around it and potentially introduce weird cases where the first byte for a value >= 128 lacks significant bit set. I didn't bother to check which direction ProtoBuf or Go varint does things, if it's different I can change it.

#167 can still technically be done as I don't verify that varints are encoded to their smallest possible representation.

@maciejhirsz
Copy link
Contributor Author

maciejhirsz commented Jun 8, 2019

I think it should be possible to use the Options trait for this, instead of a feature flag, with no extra runtime cost. The advantage would be that this setting isn't suddenly per-project global (which might matter if you use bincode for IPC and for transmiting data over the wire, with both of those having different requirements). On the flip side, that would be a breaking change (albeit only on the API surface, not affecting the ability to read old data).

Copy link
Collaborator

dtolnay left a comment

Doing it per-config rather than per-project seems better. What breaking change would be required?

@maciejhirsz
Copy link
Contributor Author

maciejhirsz commented Jun 9, 2019

I was afraid we'd have to add an enum variant or some such, but looking at how Config is constructed right now, it should be pretty straight forward to just add a method there. I'll have a go at it.

@Tolsi
Copy link

Tolsi commented Oct 14, 2019

Any updates?

@maciejhirsz
Copy link
Contributor Author

maciejhirsz commented Oct 18, 2019

@Tolsi sorry, I haven't put more time into this since, if anyone wants to pick it up from where I left it feel free, I don't think I'll have time to work on this anytime soon.

@ZoeyR
Copy link
Collaborator

ZoeyR commented Apr 16, 2020

Closing in favor of #306

@ZoeyR ZoeyR closed this Apr 16, 2020
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.

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