Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@joshtriplett Could you take a look at this PR? |
|
I read the discussion thread on internals, but unless I've missed some broader discussion on this, I feel like this change may need an associated RFC for whether we want to expose this unsafe API. I don't feel comfortable marking this as reviewed without that broader consensus. In the meantime, I'm tagging this T-libs; @rust-lang/libs, thoughts? Also, as mentioned in that thread, I do think we need a safe API that provides a variant of |
|
To be clear, nothing in these proposed changes is But yes, I didn't mean to suggest that this PR is ready to merge as is. We definitely need more discussion first, possibly including an RFC. |
|
@fintelia Ah, I see. Thanks for the clarification. I do still feel a change like this ought to have an RFC, and I do think there'd be value in having specific, less error-prone support for the "look up an entry with a borrowed key and to_owned only if needed" case, but this makes more sense now. |
|
Tirage; @joshtriplett What's the current status of the review of this PR? |
|
Current status is that this needs further discussion and possibly an RFC. |
|
@gankro, @alexcrichton, and @Amanieu you were involved on the previous iteration of this PR. Any thoughts on this version? |
|
Last I recall, the stdlib team was fine with landing this kind of thing experimentally without an RFC. |
|
Ping from triage @rust-lang/libs: This PR could use some guidance from you. |
src/libstd/collections/hash/map.rs
Outdated
| /// Unless you are in such a situation, higher-level and more foolproof APIs like | ||
| /// `get` should be preferred. | ||
| /// | ||
| /// Immutable raw entries have very limited use; you might instead want `raw_entry`. |
alexcrichton
left a comment
There was a problem hiding this comment.
Ok sorry for the delay here! We discussed this recently at a @rust-lang/libs triage meeting and our conclusion was that we're relatively confident in this and would be willing to land as unstable. We'll probably want a sign-off from other hash map experts before stabilization, but it doesn't look like that needs to block this landing to start off with!
@fintelia if you want to update with a few minor nits now I can then r+ to land this.
src/libstd/collections/hash/map.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// Search for a pre-hashed key when the hash map is known to be non-empty. |
There was a problem hiding this comment.
This function looks like it may be a copy-pasted version of the one above I think? That's fine but could the comments be removed and instead a comment placed saying it's the ssame one as above just intended for mutable versions?
src/libstd/collections/hash/map.rs
Outdated
|
|
||
| // If the hash doesn't match, it can't be this one.. | ||
| if hash == full.hash() { | ||
| if hash == full.hash() || !compare_hashes { |
There was a problem hiding this comment.
Should this comparison perhaps be swapped to avoid the comparison in the case that compare_hashes is known as a constant false?
|
@bors: r+ Thanks! |
|
📌 Commit daf5bd5 has been approved by |
Add raw_entry API to HashMap This is a continuation of #50821.
|
💔 Test failed - status-appveyor |
|
@bors retry 3 hour timeout |
Add raw_entry API to HashMap This is a continuation of #50821.
|
☀️ Test successful - status-appveyor, status-travis |
This is a continuation of #50821.