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 #306

Merged
merged 20 commits into from May 19, 2020
Merged

Varint enum tags and lengths #306

merged 20 commits into from May 19, 2020

Conversation

@ZoeyR
Copy link
Collaborator

ZoeyR commented Mar 14, 2020

Resolves #319

This is a continuation of PR #271 based on feedback in that PR. The primary change is to use a config option instead of a feature in order to use varint sequence lengths and discriminants.

@ZoeyR
Copy link
Collaborator Author

ZoeyR commented Mar 17, 2020

@dtolnay can I get a review of this?

Copy link
Collaborator

dtolnay left a comment

Hi @ZoeyR, I think #294 (review) applies here. I think this is useful but it would be better to pursue in a different library.

@dtolnay dtolnay closed this Mar 17, 2020
@ZoeyR
Copy link
Collaborator Author

ZoeyR commented Mar 17, 2020

@dtolnay I thought based on #157 (which you opened) and #271 (which you seemed agreeable towards) that this PR would be a good fit for the library. It doesn't offer all of the configuration of #294 because IMO the amount of config in #294 is a bit of a footgun.

Copy link
Contributor

strega-nil left a comment

This needs some changes, but otherwise, is a really good changeset.

src/internal.rs Outdated Show resolved Hide resolved
src/internal.rs Outdated Show resolved Hide resolved
src/internal.rs Outdated Show resolved Hide resolved
src/internal.rs Outdated Show resolved Hide resolved
src/internal.rs Outdated Show resolved Hide resolved

Ok(n | ((byte as usize) << shift))
}
}

This comment has been minimized.

@strega-nil

strega-nil Mar 18, 2020

Contributor

Add a newline here

src/internal.rs Outdated Show resolved Hide resolved
src/internal.rs Outdated Show resolved Hide resolved
maciejhirsz and others added 8 commits Jun 8, 2019
@ZoeyR ZoeyR force-pushed the ZoeyR:varint-encoding branch from 2d198ad to 477dfc5 Apr 17, 2020
ZoeyR added 2 commits Apr 17, 2020
@codecov-io
Copy link

codecov-io commented Apr 17, 2020

Codecov Report

Merging #306 into master will decrease coverage by 5.33%.
The diff coverage is 51.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #306      +/-   ##
==========================================
- Coverage   63.18%   57.84%   -5.34%     
==========================================
  Files           7        7              
  Lines         622      733     +111     
==========================================
+ Hits          393      424      +31     
- Misses        229      309      +80     
Impacted Files Coverage Δ
src/config.rs 20.51% <19.62%> (-1.08%) ⬇️
src/de/mod.rs 77.57% <88.09%> (+0.43%) ⬆️
src/ser/mod.rs 78.87% <88.23%> (+2.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8ce052...e9f9188. Read the comment docs.

@fabricedesre
Copy link

fabricedesre commented May 1, 2020

Hi! Is there anything blocking that to land?

@strega-nil
Copy link
Contributor

strega-nil commented May 1, 2020

@fabricedesre COVID, but also I'd like to add varint encoding forall integers.

strega-nil and others added 7 commits May 10, 2020
Change VarInt to support all integral types
@Zaxar163
Copy link

Zaxar163 commented May 17, 2020

When this PR will be accepted?

@ZoeyR
Copy link
Collaborator Author

ZoeyR commented May 18, 2020

@Zaxar163 once we've worked out the kinks with signed number varints.

ZoeyR and others added 3 commits May 18, 2020
[varint] add zigzag encoding for signed integers
Rename Options -> InternalOptions, OptionsExt -> Options
@ZoeyR ZoeyR merged commit 263fb94 into servo:master May 19, 2020
9 checks passed
9 checks passed
Check (stable)
Details
Check (beta)
Details
Check (nightly)
Details
Check (1.18.0)
Details
Test (stable)
Details
Test (beta)
Details
Test (nightly)
Details
Lints
Details
Code Coverage
Details
@alanjds
Copy link

alanjds commented Aug 19, 2020

Hi. When will this be released as a crate?

@ZoeyR
Copy link
Collaborator Author

ZoeyR commented Aug 20, 2020

Hi @alanjds this was release a few months ago as part of bincode 1.3

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.

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