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

Stabilize #9

Closed
wants to merge 14 commits into from
Closed

Stabilize #9

wants to merge 14 commits into from

Conversation

archer884
Copy link
Owner

@archer884 archer884 commented Jan 17, 2018

Outstanding items:

  • Validate use of unwrap and expect calls throughout
  • Remove unwrap calls from documentation
  • Consider adding Harsh::decode_single(foo) as a counterpart to Harsh::encode_single(foo)
  • Consider adding Harsh::encode_into(foo) (wording? -.-) so users can reuse existing string buffers

Note: the first part above might require fuzzing. Just saying. Hell if I know.

archer884 and others added 10 commits February 2, 2018 10:05
Configuration is no longer required, because rustfmt no longer sucks.
cargo will by default recognize tests in the tests/ folder. Use this
folder instead of a conditinal `tests` module at the root of the library
runnable with `cargo +nightly bench`
@archer884
Copy link
Owner Author

@Dr-Emann's patch adds quickcheck, which has largely resolved my questions regarding the use of unwrap and panic in the code. There was, in fact, a bug relating to attempts to decode invalid input (that is, inputs containing letters not found in the alphabet).

As a side note, if you're reading, what's the story on calls to black_box outside the benchmark iteration context? I didn't remove those, but I kind of got the idea they wouldn't do anything.

Nothing bit--just formatting changes for numbers, mostly.
@archer884
Copy link
Owner Author

I have decided against decode_into because harsh's performance is simply not marvelous enough to justify the additional API complexity.

Dr-emann's quickcheck tests uncovered a pair of input error cases that
were unaccounted for by the original impl, which had no capacity to
actually return an error anyway:

1. UTF-8 characters not in the provided hashids alphabet.
2. Encoded hashids values outside the range of u64.

These have been addressed by changing from Option to Result for the
decoding return type and by the addition of a wrapping_pow function
based on the checked_pow function of the num crate.
@archer884
Copy link
Owner Author

Leaning toward not stabilizing right now and instead releasing a 0.2.0-rc. I would prefer to present a less disjointed interface, with everything returning result values rather than some things returning options and others returning results. This is especially true in light of what I have learned recently about the performance characteristics of option values...

...Even if those can hardly be considered relevant, particularly for encoding. :)

As another side note, if I'm reading those benchmarks @Dr-Emann provided correctly, encoding takes much, much longer than decoding. Any thoughts? :|

@Dr-Emann
Copy link

Dr-Emann commented Feb 2, 2018

There's no benefit to using black_box outside the iter function, however, because I benchmarked creation using the init function, I wanted to make sure it wasn't going to do any smart inlining of salts or anything.

@Dr-Emann
Copy link

Dr-Emann commented Feb 2, 2018

My results don't look like there's a huge difference between encoding and decoding, and if anything encoding is slightly faster than decoding:

running 4 tests
test custom_creation  ... bench:       5,566 ns/iter (+/- 2,769)
test decode           ... bench:     179,099 ns/iter (+/- 61,987)
test default_creation ... bench:       3,059 ns/iter (+/- 1,301)
test encode           ... bench:     144,143 ns/iter (+/- 36,351)

with rustc version rustc 1.25.0-nightly (56733bc9f 2018-02-01) on a windows machine

@archer884
Copy link
Owner Author

archer884 commented Feb 2, 2018 via email

@archer884
Copy link
Owner Author

Looks like this is finally going to get done, but I rewrote the error code a different way.

@archer884 archer884 closed this Mar 31, 2020
@archer884 archer884 deleted the one-point-oh branch March 31, 2020 23:04
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.

None yet

2 participants