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

Add raw_entry API to HashMap #54043

Merged
merged 10 commits into from
Nov 2, 2018
Merged

Add raw_entry API to HashMap #54043

merged 10 commits into from
Nov 2, 2018

Conversation

fintelia
Copy link
Contributor

@fintelia fintelia commented Sep 7, 2018

This is a continuation of #50821.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:45] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:45] tidy error: /checkout/src/libstd/collections/hash/map.rs:1957: line longer than 100 chars
[00:04:46] some tidy checks failed
[00:04:46] 
[00:04:46] 
[00:04:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:46] 
[00:04:46] 
[00:04:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:46] Build completed unsuccessfully in 0:00:50
[00:04:46] Build completed unsuccessfully in 0:00:50
[00:04:46] make: *** [tidy] Error 1
[00:04:46] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0025d6b5
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:2dc75e69:start=1536353912255869568,finish=1536353912263740941,duration=7871373
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:02a96036
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:00cc7378
travis_time:start:00cc7378
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:284777e0
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:43:07] 
[00:43:07] warning: `[]` cannot be resolved, ignoring it...
[00:43:07]     --> libstd/collections/hash/map.rs:1862:40
[00:43:07]      |
[00:43:07] 1862 | /// See the [`HashMap::raw_entry_mut`][] docs for usage examples.
[00:43:07]      |
[00:43:07]      = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:43:07] 
[00:43:07] warning: `[]` cannot be resolved, ignoring it...
[00:43:07] warning: `[]` cannot be resolved, ignoring it...
[00:43:07]     --> libstd/collections/hash/map.rs:1906:36
[00:43:07]      |
[00:43:07] 1906 | /// See the [`HashMap::raw_entry`][] docs for usage examples.
[00:43:07]      |
[00:43:07]      = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:43:07] 
[00:43:07] warning: `[Seek::seek_relative]` cannot be resolved, ignoring it...
---
[01:11:01] travis_fold:start:test_stage1-std
travis_time:start:test_stage1-std
Testing std stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:11:02]    Compiling std v0.0.0 (file:///checkout/src/libstd)
[01:11:04] error[E0432]: unresolved import `super::RawEntry`
[01:11:04]      |
[01:11:04]      |
[01:11:04] 4273 |         use super::RawEntry::{Occupied, Vacant};
[01:11:04]      |                    ^^^^^^^^ Could not find `RawEntry` in `super`
[01:11:04] 
[01:11:09] error[E0599]: no method named `search_by` found for type `collections::hash::map::RawEntryBuilder<'_, i32, i32, collections::hash::map::RandomState>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 1908 | pub struct RawEntryBuilder<'a, K: 'a, V: 'a, S: 'a> {
[01:11:09]      | --------------------------------------------------- method `search_by` not found for this
[01:11:09] ...
[01:11:09] 4280 |         match map.raw_entry().search_by(&1) {
[01:11:09] 
[01:11:09] 
[01:11:09] error[E0599]: no method named `raw_entry_immut` found for type `collections::hash::map::HashMap<i32, i32>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 415  | pub struct HashMap<K, V, S = RandomState> {
[01:11:09]      | ----------------------------------------- method `raw_entry_immut` not found for this
[01:11:09] ...
[01:11:09] 4287 |         assert_eq!(map.raw_entry_immut().hash_with(|mut h| {
[01:11:09]      |
[01:11:09]      |
[01:11:09]      = help: did you mean `raw_entry_mut`?
[01:11:09] error[E0599]: no method named `hash` found for type `i32` in the current scope
[01:11:09]     --> libstd/collections/hash/map.rs:4288:18
[01:11:09]      |
[01:11:09]      |
[01:11:09] 4288 |             1i32.hash(&mut h);
[01:11:09]      |
[01:11:09]      = help: items from traits can only be used if the trait is in scope
[01:11:09]      = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:11:09]              `use core::hash::Hash;`
[01:11:09]              `use core::hash::Hash;`
[01:11:09] 
[01:11:09] error[E0599]: no method named `hash_by` found for type `collections::hash::map::RawEntryBuilder<'_, i32, i32, collections::hash::map::RandomState>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 1908 | pub struct RawEntryBuilder<'a, K: 'a, V: 'a, S: 'a> {
[01:11:09]      | --------------------------------------------------- method `hash_by` not found for this
[01:11:09] ...
[01:11:09] 4296 |         match map.raw_entry().hash_by(&2).search_by(&2) {
[01:11:09] 
[01:11:09] 
[01:11:09] error[E0599]: no method named `raw_entry_immut` found for type `collections::hash::map::HashMap<i32, i32>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 415  | pub struct HashMap<K, V, S = RandomState> {
[01:11:09]      | ----------------------------------------- method `raw_entry_immut` not found for this
[01:11:09] ...
[01:11:09] 4304 |         assert_eq!(map.raw_entry_immut().search_by(&2).unwrap(), (&2, &200));
[01:11:09]      |
[01:11:09]      |
[01:11:09]      = help: did you mean `raw_entry_mut`?
[01:11:09] 
[01:11:09] error[E0599]: no method named `hash_with` found for type `collections::hash::map::RawEntryBuilder<'_, i32, i32, collections::hash::map::RandomState>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 1908 | pub struct RawEntryBuilder<'a, K: 'a, V: 'a, S: 'a> {
[01:11:09]      | --------------------------------------------------- method `hash_with` not found for this
[01:11:09] ...
[01:11:09] 4308 |         match map.raw_entry().hash_with(|mut h| {
[01:11:09] 
[01:11:09] error[E0599]: no method named `hash` found for type `i32` in the current scope
[01:11:09]     --> libstd/collections/hash/map.rs:4309:18
[01:11:09]      |
[01:11:09]      |
[01:11:09] 4309 |             3i32.hash(&mut h);
[01:11:09]      |
[01:11:09]      = help: items from traits can only be used if the trait is in scope
[01:11:09]      = note: the following trait is implemented but not in scope, perhaps add a `use` for it:
[01:11:09]              `use core::hash::Hash;`
[01:11:09]              `use core::hash::Hash;`
[01:11:09] 
[01:11:09] error[E0599]: no method named `raw_entry_immut` found for type `collections::hash::map::HashMap<i32, i32>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 415  | pub struct HashMap<K, V, S = RandomState> {
[01:11:09]      | ----------------------------------------- method `raw_entry_immut` not found for this
[01:11:09] ...
[01:11:09] 4317 |         assert_eq!(map.raw_entry_immut().search_by(&3), None);
[01:11:09]      |
[01:11:09]      |
[01:11:09]      = help: did you mean `raw_entry_mut`?
[01:11:09] 
[01:11:09] error[E0599]: no method named `search_by` found for type `collections::hash::map::RawEntryBuilder<'_, i32, i32, collections::hash::map::RandomState>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 1908 | pub struct RawEntryBuilder<'a, K: 'a, V: 'a, S: 'a> {
[01:11:09]      | --------------------------------------------------- method `search_by` not found for this
[01:11:09] ...
[01:11:09] 4322 |         match map.raw_entry().search_by(&10) {
[01:11:09] 
[01:11:09] 
[01:11:09] error[E0599]: no method named `raw_entry_immut` found for type `collections::hash::map::HashMap<i32, i32>` in the current scope
[01:11:09]      |
[01:11:09]      |
[01:11:09] 415  | pub struct HashMap<K, V, S = RandomState> {
[01:11:09]      | ----------------------------------------- method `raw_entry_immut` not found for this
[01:11:09] ...
[01:11:09] 4328 |         assert_eq!(map.raw_entry_immut().hash_by(&10).search_by(&10).unwrap(), (&10, &1000));
[01:11:09]      |
[01:11:09]      |
[01:11:09]      = help: did you mean `raw_entry_mut`?
[01:11:16] error: aborting due to 11 previous errors
[01:11:16] 
[01:11:16] Some errors occurred: E0432, E0599.
[01:11:16] For more information about an error, try `rustc --explain E0432`.
[01:11:16] For more information about an error, try `rustc --explain E0432`.
[01:11:16] error: Could not compile `std`.
[01:11:16] 
[01:11:16] To learn more, run the command again with --verbose.
[01:11:16] 
[01:11:16] 
[01:11:16] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std" "--" "--quiet"
[01:11:16] 
[01:11:16] 
[01:11:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:11:16] Build completed unsuccessfully in 0:27:26
[01:11:16] Build completed unsuccessfully in 0:27:26
[01:11:16] make: *** [check] Error 1
[01:11:16] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:24240f30
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:069dffc6:start=1536783582137457295,finish=1536783582232386178,duration=94928883
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:14213f9a
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0164b185
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/e3/12/92bd3655f436aa009688e7ccb8b7581554fb64278d111f5845e79da8e618/awscli-1.16.14-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 9.9MB/s eta 0:00:01
    1% |▌                               | 20kB 1.8MB/s eta 0:00:01
    2% |▊                               | 30kB 2.1MB/s eta 0:00:01
    3% |█                               | 40kB 2.0MB/s eta 0:00:01
---
[00:47:42] 
[00:47:42] warning: `[]` cannot be resolved, ignoring it...
[00:47:42]     --> libstd/collections/hash/map.rs:1847:40
[00:47:42]      |
[00:47:42] 1847 | /// See the [`HashMap::raw_entry_mut`][] docs for usage examples.
[00:47:42]      |
[00:47:42]      = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:47:42] 
[00:47:42] warning: `[]` cannot be resolved, ignoring it...
[00:47:42] warning: `[]` cannot be resolved, ignoring it...
[00:47:42]     --> libstd/collections/hash/map.rs:1891:36
[00:47:42]      |
[00:47:42] 1891 | /// See the [`HashMap::raw_entry`][] docs for usage examples.
[00:47:42]      |
[00:47:42]      = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`
[00:47:42] 
[00:47:42] warning: `[Seek::seek_relative]` cannot be resolved, ignoring it...
---
[01:27:34] travis_fold:end:stage0-linkchecker

[01:27:34] travis_time:end:stage0-linkchecker:start=1536875406008140072,finish=1536875408560221231,duration=2552081159

[01:29:29] std/collections/hash_map/enum.RawEntryMut.html:5: broken link - std/collections/hash_map/struct.Entry.html
[01:30:32] thread 'main' panicked at 'found some broken links', tools/linkchecker/main.rs:49:9
[01:30:32] 
[01:30:32] 
[01:30:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/linkchecker" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc"
[01:30:32] expected success, got: exit code: 101
[01:30:32] expected success, got: exit code: 101
[01:30:32] 
[01:30:32] 
[01:30:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:30:32] Build completed unsuccessfully in 0:42:04
[01:30:32] make: *** [check] Error 1
[01:30:32] Makefile:58: recipe for target 'check' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0acc4040
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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 @TimNN. (Feature Requests)

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:20:46] travis_fold:end:stage0-linkchecker

[01:20:46] travis_time:end:stage0-linkchecker:start=1536884988639382824,finish=1536884990980139656,duration=2340756832

[01:23:08] std/collections/hash_map/enum.RawEntryMut.html:5: broken link - std/collections/hash_map/struct.Entry.html
[01:24:35] thread 'main' panicked at 'found some broken links', tools/linkchecker/main.rs:49:9
[01:24:35] 
[01:24:35] 
[01:24:35] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/linkchecker" "/checkout/obj/build/x86_64-unknown-linux-gnu/doc"
[01:24:35] expected success, got: exit code: 101
[01:24:35] expected success, got: exit code: 101
[01:24:35] 
[01:24:35] 
[01:24:35] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:24:35] Build completed unsuccessfully in 0:40:25
[01:24:35] Makefile:58: recipe for target 'check' failed
[01:24:35] make: *** [check] Error 1
141748 ./obj/build/x86_64-unknown-linux-gnu/stage1-rustc/x86_64-unknown-linux-gnu/release
134588 ./obj/build/bootstrap/debug/incremental/bootstrap-j9sjo2qxwegl
134588 ./obj/build/bootstrap/debug/incremental/bootstrap-j9sjo2qxwegl
134584 ./obj/build/bootstrap/debug/incremental/bootstrap-j9sjo2qxwegl/s-f4s12l7dz9-9c2pu6-1jtu7ee7y4raz
134004 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu
134000 ./obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release
130456 ./obj/cores
126320 ./obj/build/x86_64-unknown-linux-gnu/stage1-std
---
40952 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc/release/build
39272 ./obj/build/x86_64-unknown-linux-gnu/doc/book
38120 ./obj/build/x86_64-unknown-linux-gnu/stage0-std
37756 ./src/tools/lldb/www
37404 ./obj/brintf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0732e63a
travis_time:start:0732e63a
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1080a3a3
$ dmesg | grep -i kill

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 @TimNN. (Feature Requests)

@fintelia
Copy link
Contributor Author

@joshtriplett Could you take a look at this PR?

@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 14, 2018
@joshtriplett
Copy link
Member

joshtriplett commented Sep 14, 2018

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 entry that works with a borrowed key and subsequently calls to_owned if filling in an empty entry. It'd be unfortunate if doing that required calling an unsafe API.

@fintelia
Copy link
Contributor Author

To be clear, nothing in these proposed changes is unsafe in the Rust sense: there is no way to trigger undefined behavior or memory safety violations using this interface. Further, any violation of HashMap's invariants that is possible with this design is also currently possible with safe stable Rust using interior mutability/dishonest implementations of Hash or Eq.

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.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 15, 2018

@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.

@XAMPPRocky
Copy link
Member

Tirage; @joshtriplett What's the current status of the review of this PR?

@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2018
@joshtriplett
Copy link
Member

Current status is that this needs further discussion and possibly an RFC.

@fintelia
Copy link
Contributor Author

fintelia commented Oct 4, 2018

@gankro, @alexcrichton, and @Amanieu you were involved on the previous iteration of this PR. Any thoughts on this version?

@Gankra
Copy link
Contributor

Gankra commented Oct 4, 2018

Last I recall, the stdlib team was fine with landing this kind of thing experimentally without an RFC.

@TimNN TimNN added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 9, 2018
@TimNN
Copy link
Contributor

TimNN commented Oct 9, 2018

Ping from triage @rust-lang/libs: This PR could use some guidance from you.

/// 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`.
Copy link
Member

Choose a reason for hiding this comment

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

raw_entry_mut?

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

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.

@@ -488,6 +489,57 @@ fn search_hashed_nonempty<K, V, M, F>(table: M, hash: SafeHash, mut is_match: F)
}
}

/// Search for a pre-hashed key when the hash map is known to be non-empty.
Copy link
Member

Choose a reason for hiding this comment

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

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?

@@ -476,7 +477,7 @@ fn search_hashed_nonempty<K, V, M, F>(table: M, hash: SafeHash, mut is_match: F)
}

// If the hash doesn't match, it can't be this one..
if hash == full.hash() {
if hash == full.hash() || !compare_hashes {
Copy link
Member

Choose a reason for hiding this comment

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

Should this comparison perhaps be swapped to avoid the comparison in the case that compare_hashes is known as a constant false?

@alexcrichton alexcrichton added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 31, 2018
@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Oct 31, 2018

📌 Commit daf5bd5 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 31, 2018
@bors
Copy link
Contributor

bors commented Nov 1, 2018

⌛ Testing commit daf5bd5 with merge 14331aa...

bors added a commit that referenced this pull request Nov 1, 2018
Add raw_entry API to HashMap

This is a continuation of #50821.
@bors
Copy link
Contributor

bors commented Nov 1, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2018
@kennytm
Copy link
Member

kennytm commented Nov 1, 2018

@bors retry

3 hour timeout

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2018
@bors
Copy link
Contributor

bors commented Nov 2, 2018

⌛ Testing commit daf5bd5 with merge e800988...

bors added a commit that referenced this pull request Nov 2, 2018
Add raw_entry API to HashMap

This is a continuation of #50821.
@bors
Copy link
Contributor

bors commented Nov 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing e800988 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants