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

Review #1

Closed
dhardy opened this issue Oct 12, 2019 · 7 comments · Fixed by #2
Closed

Review #1

dhardy opened this issue Oct 12, 2019 · 7 comments · Fixed by #2

Comments

@dhardy
Copy link
Member

dhardy commented Oct 12, 2019

New code deserves review — in this case, since this is a new repository, I posted the code directly instead of via PR.

The initial four commits are exactly those found in rust-random/rand#554 (just rebased).

Perhaps @burdges or @vks would care to review based on the state of this repository?

@dhardy
Copy link
Member Author

dhardy commented Oct 12, 2019

Update: I pushed two commits to fix the Miri issues and fix a portability issue (not covered by tests).

@burdges
Copy link

burdges commented Oct 12, 2019

I donno if this needs a separate crate really.. It's only a few lines to do yourself.

I've written a few methods called make_rng but normally they needed to take something else into account.

@dhardy
Copy link
Member Author

dhardy commented Oct 12, 2019

There's more than a few lines here. It also offers another feature: a portable hasher. And it's still a very light crate. I don't personally have any use for this, but I think it's interesting enough to be worth publishing (given that I already did most of the work).

@dhardy
Copy link
Member Author

dhardy commented Oct 15, 2019

Tests pass now. We have no eager reviewers (not that I expected anything else for this crate), so it makes sense to go ahead and publish 0.1.0.

@vks
Copy link

vks commented Oct 15, 2019

Looks good! I only have minor comments:

  • The following names might be more idiomatic: new_with_keys -> from_keys, make_rng -> into_rng.
  • "This might break the hasher's security." should probably be added to the docs of SipHasher::make_rng.
  • "It is statistically high-quality, passing practrand tests to at least 4 TiB." should probably be added to the docs of SipRng.
  • The documentation of the rand_seeder module should probably be extended to include the information from the README. This would help a lot when looking at the library on docs.rs.
  • The links in the README are somewhat broken.

@vks
Copy link

vks commented Oct 15, 2019

The docs upload makes Travis fail and should probably be removed:

$ travis-cargo --only nightly doc-upload
uploading docs...
Cloning into 'ghp-import'...
remote: Enumerating objects: 308, done.
remote: Total 308 (delta 0), reused 0 (delta 0), pack-reused 308
Receiving objects: 100% (308/308), 77.66 KiB | 1.32 MiB/s, done.
Resolving deltas: 100% (166/166), done.
remote: Invalid username or password.
fatal: Authentication failed for 'https://XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX@github.com/rust-random/rand.git/'

@dhardy
Copy link
Member Author

dhardy commented Oct 15, 2019

Ah, thanks for the review; I assumed you were not interested. I will get to this shortly (and maybe deprecate the old method names).

dhardy added a commit that referenced this issue Oct 31, 2019
@dhardy dhardy mentioned this issue Oct 31, 2019
@dhardy dhardy closed this as completed in #2 Nov 13, 2019
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 a pull request may close this issue.

3 participants