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

std: Stabilize custom hasher support in HashMap #31081

Merged
merged 1 commit into from
Jan 26, 2016

Conversation

alexcrichton
Copy link
Member

This commit implements the stabilization of the custom hasher support intended
for 1.7 but left out due to some last-minute questions that needed some
decisions. A summary of the actions done in this PR are:

Stable

  • std::hash::BuildHasher
  • BuildHasher::Hasher
  • BuildHasher::build_hasher
  • std::hash::BuildHasherDefault
  • HashMap::with_hasher
  • HashMap::with_capacity_and_hasher
  • HashSet::with_hasher
  • HashSet::with_capacity_and_hasher
  • std::collections::hash_map::RandomState
  • RandomState::new

Deprecated

  • std::collections::hash_state
  • std::collections::hash_state::HashState - this trait was also moved into
    std::hash with a reexport here to ensure that we can have a blanket impl to
    prevent immediate breakage on nightly. Note that this is unstable in both
    location.
  • HashMap::with_hash_state - renamed
  • HashMap::with_capacity_and_hash_state - renamed
  • HashSet::with_hash_state - renamed
  • HashSet::with_capacity_and_hash_state - renamed

Closes #27713

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @aturon

cc @rust-lang/libs

nominating for a beta-backport

@rust-highfive rust-highfive assigned aturon and unassigned nikomatsakis Jan 21, 2016
@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 21, 2016
@Gankra
Copy link
Contributor

Gankra commented Jan 21, 2016

@shepmaster
Copy link
Member

Everything here seems reasonable and as was discussed. 👍

pub fn with_hash_state(hash_state: S) -> HashMap<K, V, S> {
HashMap::with_hasher(hash_state)
}

/// Creates an empty HashMap with space for at least `capacity`
Copy link
Member

Choose a reason for hiding this comment

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

Should we give the same warning about DoS here?

Copy link
Member

Choose a reason for hiding this comment

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

Ooops, github just had it collapsed, nevermind.

@aturon
Copy link
Member

aturon commented Jan 22, 2016

LGTM -- though I did leave one question about the actual API. I suspect I'm just failing to remember one of the design constraints here.

@alexcrichton
Copy link
Member Author

@aturon I've updated with removing the old and misleading HashState documentation, and as @shepmaster mentioned the &self is because these are just hash factories as opposed to hashers themselves. The &self is required to work because hashers need to be created in the fn get(&self) function on a HashMap.

@sfackler
Copy link
Member

This looks good to go, right?

@aturon
Copy link
Member

aturon commented Jan 26, 2016

@alexcrichton corrected the wrong docs problem :) I still find the docs super misleading, but we can fix in post if you prefer. That's my only complaint.

@alexcrichton
Copy link
Member Author

I'm fine changing the docs, but I'm not really quite sure what to change them to? Do you have something in mind @aturon ?

@@ -190,6 +191,79 @@ pub trait Hasher {
}
}

/// A trait representing an object which maintains state to create new instances
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: nix this sentence.

@aturon
Copy link
Member

aturon commented Jan 26, 2016

@alexcrichton Left some minimal suggestions. With that r=me.

This commit implements the stabilization of the custom hasher support intended
for 1.7 but left out due to some last-minute questions that needed some
decisions. A summary of the actions done in this PR are:

Stable

* `std::hash::BuildHasher`
* `BuildHasher::Hasher`
* `BuildHasher::build_hasher`
* `std::hash::BuildHasherDefault`
* `HashMap::with_hasher`
* `HashMap::with_capacity_and_hasher`
* `HashSet::with_hasher`
* `HashSet::with_capacity_and_hasher`
* `std::collections::hash_map::RandomState`
* `RandomState::new`

Deprecated

* `std::collections::hash_state`
* `std::collections::hash_state::HashState` - this trait was also moved into
  `std::hash` with a reexport here to ensure that we can have a blanket impl to
  prevent immediate breakage on nightly. Note that this is unstable in both
  location.
* `HashMap::with_hash_state` - renamed
* `HashMap::with_capacity_and_hash_state` - renamed
* `HashSet::with_hash_state` - renamed
* `HashSet::with_capacity_and_hash_state` - renamed

Closes rust-lang#27713
@alexcrichton
Copy link
Member Author

@bors: r=aturon 1fa0be2

bors added a commit that referenced this pull request Jan 26, 2016
This commit implements the stabilization of the custom hasher support intended
for 1.7 but left out due to some last-minute questions that needed some
decisions. A summary of the actions done in this PR are:

Stable

* `std::hash::BuildHasher`
* `BuildHasher::Hasher`
* `BuildHasher::build_hasher`
* `std::hash::BuildHasherDefault`
* `HashMap::with_hasher`
* `HashMap::with_capacity_and_hasher`
* `HashSet::with_hasher`
* `HashSet::with_capacity_and_hasher`
* `std::collections::hash_map::RandomState`
* `RandomState::new`

Deprecated

* `std::collections::hash_state`
* `std::collections::hash_state::HashState` - this trait was also moved into
  `std::hash` with a reexport here to ensure that we can have a blanket impl to
  prevent immediate breakage on nightly. Note that this is unstable in both
  location.
* `HashMap::with_hash_state` - renamed
* `HashMap::with_capacity_and_hash_state` - renamed
* `HashSet::with_hash_state` - renamed
* `HashSet::with_capacity_and_hash_state` - renamed

Closes #27713
@bors
Copy link
Contributor

bors commented Jan 26, 2016

⌛ Testing commit 1fa0be2 with merge a9e139b...

@bors bors merged commit 1fa0be2 into rust-lang:master Jan 26, 2016
@alexcrichton
Copy link
Member Author

Marking as accepted for beta as libs talked about this awhile ago

@alexcrichton alexcrichton deleted the stabilize-hasher branch January 27, 2016 17:04
@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants