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

Fix EncodeHex/DecodeHex #46

Merged
merged 4 commits into from
Jun 26, 2018
Merged

Fix EncodeHex/DecodeHex #46

merged 4 commits into from
Jun 26, 2018

Conversation

dolmen
Copy link
Collaborator

@dolmen dolmen commented Jun 26, 2018

Fixes for the unfortunately merged #41:

  • add testcase for encoding (the testcase in Hex #41 only checks roundtrip but this doesn't enforce compatibility with other implementations or protects against change in the encoding)
  • check bounds of decoded numbers in DecodeHex (to avoid crashing)
  • faster EncodeHex
  • faster DecodeHex
  • add documentation about the encoding (important as it is quite awkward: each hex digit is encoded as an int in range [16, 31])

Note: this is not an endorsement of the EncodeHex/DecodeHex feature. Instead I would prefer it would be completely removed because it is quite awkward.

Cc: @brandoneprice31

dolmen added 4 commits June 26, 2018 14:02
Because verifying roundtrip is not enough to ensure no regression.
Cc: @brandoneprice31
- check that decoded numbers are in range
- much much faster implementation
@speps speps merged commit ad304c6 into speps:master Jun 26, 2018
@speps
Copy link
Owner

speps commented Jun 26, 2018

Thanks a lot.

@speps
Copy link
Owner

speps commented Jun 26, 2018

I agree it should ideally go. As I said in the other thread, let me know if you want more rights on this repo and get it to 2.0 (breaking changes, remove fluff, etc.).

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