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

Remove BuildHasher requirement from raw entry APIs. #123

Merged
merged 1 commit into from Nov 7, 2019

Conversation

@goffrie
Copy link
Contributor

goffrie commented Oct 15, 2019

This makes it possible to use HashMap with S=(), which forces the user to always supply hashes manually. This is an alternative to making the key type unhashable.

I'm not sure if this change is desirable but I'm putting it up for discussion anyway :)

@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Nov 3, 2019

Sorry about the late response, I somehow missed this PR.

Could you explain your use case for this functionality? Why can't you just use the default hasher? If your key is not Hash then you'll need to provide hashes manually anyways through RawEntry.

@goffrie

This comment has been minimized.

Copy link
Contributor Author

goffrie commented Nov 3, 2019

So, my use case is that I have a HashMap<usize, V> where usize is indexing into some other table and fetching the actual keys from that table. As such, I'm always using raw entry APIs to provide the hash/comparison function myself. This works fine, but it means that it's possible for me to accidentally use an API that hashes my usize and create wrong behaviour that way. With this patch, I can instead make my map HashMap<usize, V, ()>, and it becomes statically impossible for the hashmap to do that.

It's also possible for me to do this today using a wrapper type HashMap<NonHashable<usize>, V> where NonHashable doesn't implement Hash, but that seems more cumbersome to me.

If you have other suggestions I'm open to them too :)

@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Nov 3, 2019

That actually sounds pretty reasonable.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 3, 2019

📌 Commit 3777125 has been approved by Amanieu

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 3, 2019

⌛️ Testing commit 3777125 with merge 1eb481d...

bors added a commit that referenced this pull request Nov 3, 2019
Remove BuildHasher requirement from raw entry APIs.

This makes it possible to use HashMap with `S=()`, which forces the user to always supply hashes manually. This is an alternative to making the key type unhashable.

I'm not sure if this change is desirable but I'm putting it up for discussion anyway :)
@goffrie

This comment has been minimized.

Copy link
Contributor Author

goffrie commented Nov 3, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 3, 2019

💔 Test failed - checks-travis

@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Nov 3, 2019

Could you fix the compile errors? I know they're not your fault, but they should be easy to fix.

@Amanieu

This comment has been minimized.

Copy link
Collaborator

Amanieu commented Nov 7, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 7, 2019

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: #128
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 7, 2019

📌 Commit 3777125 has been approved by Amanieu

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 7, 2019

⌛️ Testing commit 3777125 with merge f2de794...

bors added a commit that referenced this pull request Nov 7, 2019
Remove BuildHasher requirement from raw entry APIs.

This makes it possible to use HashMap with `S=()`, which forces the user to always supply hashes manually. This is an alternative to making the key type unhashable.

I'm not sure if this change is desirable but I'm putting it up for discussion anyway :)
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 7, 2019

☀️ Test successful - checks-travis
Approved by: Amanieu
Pushing f2de794 to master...

@bors bors merged commit 3777125 into rust-lang:master Nov 7, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@goffrie

This comment has been minimized.

Copy link
Contributor Author

goffrie commented Nov 7, 2019

ah, I missed that. Thanks!

@goffrie goffrie deleted the goffrie:raw-entry-no-build-hasher branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.