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

Clarify documentation of hash::SipHasher #32091

Merged
merged 3 commits into from
Mar 10, 2016

Conversation

dirk
Copy link
Contributor

@dirk dirk commented Mar 7, 2016

The docs were making assertions/recommendations they shouldn't have. This clarifies them and adds some helpful links.

Fixes #32043.

r? @sfackler

@sfackler
Copy link
Member

sfackler commented Mar 7, 2016

cc @rust-lang/libs

@sfackler
Copy link
Member

sfackler commented Mar 7, 2016

r? @steveklabnik

@bluss
Copy link
Member

bluss commented Mar 7, 2016

Since we want to change so that this is not the default hash algorithm for hashmaps, maybe we shouldn't say anything about how it's used? It's not really relevant (as documentation for this API item).

Below that, "cryptographically strong" by itself is misleading, leads people to think about cryptographic hash functions (this is not one), so we should remove that.

@dirk
Copy link
Contributor Author

dirk commented Mar 7, 2016

@bluss:

Since we want to change so that this is not the default hash algorithm for hashmaps, maybe we shouldn't say anything about how it's used? It's not really relevant (as documentation for this API item).

I think it's worth keeping that in there for reference until such time that it is no longer the default (as I'm guessing that at least some people have wanted to know what the default hash function is).

Below that, "cryptographically strong" by itself is misleading, leads people to think about cryptographic hash functions (this is not one), so we should remove that.

Good point! Updated in 054196d.

/// such as `rand::Rng`.
/// This is the default hashing function used by standard library (eg.
/// [`collections::HashMap`](../collections/struct.HashMap.html) uses it by
/// default).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link won't work in core's docs. Not sure we have any way to link this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluss: AFAIK it will work just fine on doc.rust-lang.org, which is where I believe the majority of users view these docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to work both here and here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluss: Oh, very true. I removed the link. 😄

dirk added 2 commits March 6, 2016 20:03
The docs were making assertions/recommendations they shouldn't
have. This clarifies them and adds some helpful links.

Fixes rust-lang#32043.
@dirk dirk force-pushed the dirk/siphasher-docs-clarification branch from 2937a41 to 054196d Compare March 7, 2016 04:03
/// runs at good speed (competitive with spooky and city) and permits
/// strong _keyed_ hashing. Key your hashtables from a strong RNG,
/// such as `rand::Rng`.
/// This is the default hashing function used by standard library (eg.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this could say "this is currently the default" to emphasize that it may change over time?

@steveklabnik
Copy link
Member

r? @alexcrichton

I will gladly accept whatever you deem is best here.

@dirk
Copy link
Contributor Author

dirk commented Mar 8, 2016

I'll get the edits in tonight so y'all can review tomorrow!

On Mar 8, 2016, at 10:25 AM, Steve Klabnik [email protected] wrote:

r? @alexcrichton

I will gladly accept whatever you deem is best here.


Reply to this email directly or view it on GitHub.

@dirk
Copy link
Contributor Author

dirk commented Mar 9, 2016

@alexcrichton: Fixed in 46dc35e. 😄

@alexcrichton
Copy link
Member

@bors: r+ 46dc35e rollup

Thanks!

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Mar 10, 2016
…ion, r=alexcrichton

Clarify documentation of hash::SipHasher

The docs were making assertions/recommendations they shouldn't have. This clarifies them and adds some helpful links.

Fixes rust-lang#32043.

r? @sfackler
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Mar 10, 2016
…ion, r=alexcrichton

Clarify documentation of hash::SipHasher

The docs were making assertions/recommendations they shouldn't have. This clarifies them and adds some helpful links.

Fixes rust-lang#32043.

r? @sfackler
bors added a commit that referenced this pull request Mar 10, 2016
Rollup of 8 pull requests

- Successful merges: #31830, #32091, #32125, #32136, #32147, #32148, #32149, #32150
- Failed merges:
@bors bors merged commit 46dc35e into rust-lang:master Mar 10, 2016
@dirk dirk deleted the dirk/siphasher-docs-clarification branch March 23, 2016 07:00
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.

6 participants