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

Encoding improvements #116

Closed

Conversation

tankyleo
Copy link
Contributor

Miscellaneous collection of improvements to hex encoding, please see the commit messages for further details:

  • (API Break) BytesToHexIter can now yield uppercase characters.
  • (API Break) BufEncoder now enforces uniform case hex encoding.
  • Performance improvements to DisplayHex and BytesToHexIter.

For a given byte, `Table::byte_to_hex` returns the two hex characters in
a `[u8; 2]`, not an `ArrayString<2>`.

This gives callers the freedom to interpret the return value as a `&str`
via `str::from_utf8` or a collection of `char`'s.

`as_hex` now runs in 5x fewer "Estimated Cycles" on
[email protected].
A right shift of 4 on a u8 will never panic, and `usize::from` on u8 is
`as usize` under the hood.
Deletes `fn byte_to_hex_chars`.

`BytesToHexIter` now runs in 2x fewer "Estimated Cycles" on
[email protected].
Allows `BytesToHexIter` to iterate over references.
`BytesToHexIter` can now yield uppercase characters.
The API now enforces that all hex strings encoded with `BufEncoder`
contain characters of the same case.
@tankyleo
Copy link
Contributor Author

This PR is ready for review, but it breaks #113, so I mark it as draft until #113 is merged, happy to be patient.

Looks like we are doing a major release soon, so I thought the API breaks might fit in nicely.

Please see https://github.com/tankyleo/hex-benchmarks for how I got the "Estimated Cycles" numbers in the commit messages.

Thank you for your time, I appreciate the reviews.

@tcharding
Copy link
Member

Thanks for the contribution. Can you please break this up into separate PRs, each doing a single thing, and justify why each PR is needed. We are getting pretty close to stabalizing this crate so we don't want to just merge any functionality willy-nilly. Thanks.

@tankyleo
Copy link
Contributor Author

@tcharding thank you for the feedback.

Should each PR be its own separate branch from master, or can they all be from the same branch ?

Thank you.

@tcharding
Copy link
Member

For each PR create a separate branch. If one PR needs patches (commits) from another PR just include them but mark the PR as draft until the first one merges, then you can rebase. You can comment "draft until #xyz gets merged" if you like.

@tankyleo tankyleo closed this Oct 13, 2024
@tankyleo tankyleo deleted the oct-encoding-improvements branch October 19, 2024 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants