Skip to content

Change HashMap to be hasher-generic #12544

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

Merged
merged 2 commits into from
Feb 28, 2014
Merged

Change HashMap to be hasher-generic #12544

merged 2 commits into from
Feb 28, 2014

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Feb 25, 2014

This PR allows HashMaps to work with custom hashers. Also with this patch are:

  • a couple generic implementations of Hash for a variety of types.
  • added Default, Clone impls to the hashers.
  • added a HashMap::with_hasher() constructor.

@emberian
Copy link
Member

looks fine besides the doc nits.

self.pop_internal(hash, k)
}
}

impl<K: Hash + Eq, V> HashMap<K, V> {
impl<K: Hash<SipState> + Eq, V> HashMap<K, V, SipHasher> {
Copy link
Member

Choose a reason for hiding this comment

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

In theory, this signature doesn't need to change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you're right it shouldn't need to be changed.

@alexcrichton
Copy link
Member

I'm a little uneasy enabling default type parameters in lots more crates, was this done to fix compile errors or to make hashing these types more robust? I would rather rely on deriving(Hash) for now if possible.

@alexcrichton
Copy link
Member

I'm a little worried about documentation/readability; these signatures have reached the point that they're completely opaque. Just a concern, probably shouldn't block this.

@erickt
Copy link
Contributor Author

erickt commented Feb 25, 2014

@alexcrichton: A couple things:

  • First, with Change HashMap to be hasher-generic #12544, #[deriving(Hash)] no longer supports hashing with an arbitrary hasher, it only works with SipHash. Unfortunately that means if you want to play with arbitrary hashers, you need to manually implement Hash yourself.
  • Second, there isn't really a good way for a user that's turned on default type parameters to hash types like Url, Path, and Uuid if there isn't a generic Hash impl for Url. So I added explicit generic impls for those types.
  • Third, I agree these type parameters are getting a bit opaque. Fortunately though only the 4 typarams need to be used when you want to support a generic HashMap. End users will typically only have to deal with HashMap<K, V>. To help with hashmap.rs readability, the simplest thing we could do is give the S and H typarams better names. Replacing S with HashState and H with Hasher would be a bit clearer of their purpose. Ideally though we would clone @nikomatsakis a couple times and get all of them to implement associated items and move the S type into the H hasher type :)

@erickt
Copy link
Contributor Author

erickt commented Feb 27, 2014

@cmr: updated the pr with your doc fixes.

@alexcrichton: I factored out making a bunch of types hasher-generic into a future PR. I've left in a hasher-generic Default impl for HashMap though so that we can use encodable/decodable to serialize hasher-generic HashMaps. Or do you still want that delayed until we enable default typarams for everyone?

bors added a commit that referenced this pull request Feb 28, 2014
This PR allows `HashMap`s to work with custom hashers. Also with this patch are:

* a couple generic implementations of `Hash` for a variety of types.
* added `Default`, `Clone` impls to the hashers.
* added a `HashMap::with_hasher()` constructor.
@bors bors closed this Feb 28, 2014
@bors bors merged commit adeb730 into rust-lang:master Feb 28, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
Show proc-macro loading errors in unresolved-proc-macro diagnostics

This should help out people to potentially figure out the problem without having to check the logs
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 18, 2024
…ffix

Fixes rust-lang#12544.

- don't report an item name if it consists only of a prefix from `allowed-prefixes` list and a module name (e.g. `AsFoo` in module `foo`).
- configured by `allowed-prefixes` config entry
- prefixes allowed by default: [`to`, `from`, `into`, `as`, `try_into`, `try_from`]
- update docs
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 18, 2024
…ns-in-module-name-repetitions, r=Jarcho

[`module_name_repetition`] Recognize common prepositions

Fixes rust-lang#12544

changelog: [`module_name_repetition`]: don't report an item name if it consists only of a prefix from `allowed-prefixes` list and a module name (e.g. `AsFoo` in module `foo`). Prefixes allowed by default: [`to`, `from`, `into`, `as`, `try_into`, `try_from`]
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.

4 participants