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

Update hashbrown to 0.8.1 #70052

Merged
merged 4 commits into from
Aug 7, 2020
Merged

Update hashbrown to 0.8.1 #70052

merged 4 commits into from
Aug 7, 2020

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Mar 16, 2020

This update includes:

Fixes #28481

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 Mar 16, 2020
@Centril
Copy link
Contributor

Centril commented Mar 16, 2020

@bors rollup=never

@M-PC

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

Looking over the diff in the PR you linked, this adds two uses of specialization to hashbrown which I believe carry over into std. I believe we are currently trying to vet specialization before adding more copies by ccing lang folks.

I am unsure who the "experts" here are, but let's try @nikomatsakis and perhaps @Centril first?

@Centril
Copy link
Contributor

Centril commented Mar 17, 2020

I am unsure who the "experts" here are, but let's try @nikomatsakis and perhaps @Centril first?

cc @matthewjasper

@Centril
Copy link
Contributor

Centril commented Mar 17, 2020

Aside: reliance on default fn as a syntax here is implemented in a problematic way. The right way to do it is to delay parsing in a macro or move the default fns to a separate external module that is cfged. The way this is done in the diffs above, we will have a headache if we ever decide to change the syntax for specialization, so this should be fixed.

@Amanieu
Copy link
Member Author

Amanieu commented Mar 17, 2020

Could you give an example of how delayed parsing with a macro would work?

Is it something like this?

#[cfg(feature = "nightly")]
macro_rules! default_fn {
	($($x:tt)*) => {
        default $($x)*
    }
}
#[cfg(not(feature = "nightly"))]
macro_rules! default_fn {
	($($x:tt)*) => {
        $($x)*
    }
}

@Centril
Copy link
Contributor

Centril commented Mar 17, 2020

@Amanieu Example due to @petrochenkov #65860 (comment).

@Amanieu
Copy link
Member Author

Amanieu commented Mar 17, 2020

I started a PR at rust-lang/hashbrown#147, let me know if any other changes are needed.

@matthewjasper
Copy link
Contributor

min_specialization is in the next nightly.
I would prefer either min_specialization to be used if possible or a sepatate feature flag if not.

@Amanieu
Copy link
Member Author

Amanieu commented Mar 20, 2020

I tried compiling with min_specialization, but got these errors:

error: cannot specialize on trait `core::marker::Copy`
    --> src/raw/mod.rs:1047:1
     |
1047 | / impl<T: Copy> RawTableClone for RawTable<T> {
1048 | |     #[cfg_attr(feature = "inline-more", inline)]
1049 | |     unsafe fn clone_from_spec(&mut self, source: &Self, _on_panic: impl FnMut(&mut Self)) {
1050 | |         source
...    |
1060 | |     }
1061 | | }
     | |_^

error: cannot specialize on trait `core::cmp::Eq`
   --> src/map.rs:225:9
    |
225 | /         impl<K: Clone, V: Clone, S> HashClone<S> for HashMap<K, V, S>
226 | |         where
227 | |             K: Eq + Hash,
228 | |             S: BuildHasher,
...   |
234 | |             }
235 | |         }
    | |_________^

error: cannot specialize on trait `core::hash::Hash`
   --> src/map.rs:225:9
    |
225 | /         impl<K: Clone, V: Clone, S> HashClone<S> for HashMap<K, V, S>
226 | |         where
227 | |             K: Eq + Hash,
228 | |             S: BuildHasher,
...   |
234 | |             }
235 | |         }
    | |_________^

error: cannot specialize on trait `core::hash::BuildHasher`
   --> src/map.rs:225:9
    |
225 | /         impl<K: Clone, V: Clone, S> HashClone<S> for HashMap<K, V, S>
226 | |         where
227 | |             K: Eq + Hash,
228 | |             S: BuildHasher,
...   |
234 | |             }
235 | |         }
    | |_________^

@Amanieu
Copy link
Member Author

Amanieu commented Mar 20, 2020

If I'm understanding the min_specialization documentation correctly, this means that we need to add #[rustc_specialization_trait] on Copy, Eq, Hash and BuildHasher. Is that correct?

@bors
Copy link
Contributor

bors commented Mar 27, 2020

☔ The latest upstream changes (presumably #69470) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon 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 Apr 6, 2020
@Mark-Simulacrum
Copy link
Member

@matthewjasper Could you perhaps help @Amanieu with the answer to the above question?

@nikomatsakis
Copy link
Contributor

I'm trying to remember under what conditions adding rustc_specialization_trait makes sense. I'm a bit nervous about us adding it loosely. But I guess I'll wait until @matthewjasper weighs in -- or else I should go refresh my memory perhaps of what precisely it means and when it's ok to use it. =)

@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

@Amanieu Could we drop the specializations meanwhile to land the other bits sooner?

@Amanieu
Copy link
Member Author

Amanieu commented Apr 10, 2020

The specializations are really the only significant improvement that is relevant to libstd. There's not much point in upgrading otherwise.

@Centril
Copy link
Contributor

Centril commented Apr 10, 2020

r? @matthewjasper

@matthewjasper
Copy link
Contributor

rustc_specialization_trait can't be added to stable traits since it prevents implementations in crates which don't have feature(min_specialization) or feature(specialization).
Copy might get rustc_unsafe_specialization_marker on it temporarily, but it's not really correct and i don't want crates outside of std rely on it when it's removed.

@joelpalmer joelpalmer 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2020
bors added a commit to rust-lang/hashbrown that referenced this pull request Apr 25, 2020
Future-proof specialization code

As per @Centril's [comment](rust-lang/rust#70052 (comment))
@Amanieu
Copy link
Member Author

Amanieu commented Apr 27, 2020

Hmm, upon further review I do believe that the specialization on Eq + Hash + BuildHasher is unsound. I reverted it in hashbrown 0.7.2.

The other one uses Copy, which could be made to work with rustc_unsafe_specialization_marker. I think it should be fine to rely on this since:

  • this is only used when the "nightly" feature of hashbrown is used. It is acceptable for compiler updates to break people who have opted into using nightly features.
  • the use of specialization is an implementation detail and can be reverted at any time without any change to the API.
  • hashbrown is maintained by the libs team and can quickly react to any changes in the compiler regarding specialization.

@bors
Copy link
Contributor

bors commented Jul 23, 2020

💔 Test failed - checks-actions

@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 Jul 23, 2020
@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum Mark-Simulacrum 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 Aug 2, 2020
@Amanieu
Copy link
Member Author

Amanieu commented Aug 7, 2020

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Aug 7, 2020

📌 Commit e46bb17 has been approved by Mark-Simulacrum

@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 Aug 7, 2020
@bors
Copy link
Contributor

bors commented Aug 7, 2020

⌛ Testing commit e46bb17 with merge 8b26609...

@bors
Copy link
Contributor

bors commented Aug 7, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Mark-Simulacrum
Pushing 8b26609 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 7, 2020
@bors bors merged commit 8b26609 into rust-lang:master Aug 7, 2020
artemmukhin added a commit to intellij-rust/intellij-rust that referenced this pull request Oct 6, 2020
artemmukhin added a commit to intellij-rust/intellij-rust that referenced this pull request Oct 14, 2020
bors bot added a commit to intellij-rust/intellij-rust that referenced this pull request Oct 14, 2020
6258: Update HashMap/HashSet pretty-printers to Rust 1.47 r=Undin a=ortem

Fixes #6198

The corresponding PRs in rustc:
rust-lang/rust#76458
rust-lang/rust#70052

Besides these changes from the upstreamed pretty-printers, I've added `GetTypedefedType` (LLDB) and `strip_typedefs` (GDB) calls to resolve key and value types completely. Without these calls, LLDB doesn't show the actual type and so CLion fails to show the content of key/value pairs.

For example,
with `GetTypedefedType`:
```
(lldb) frame variable hm[0]
((i32, alloc::string::String)) hm[0] = { ... }
```

and without:
```
(lldb) frame variable hm[0]
(T) hm[0] = { ... }
```


**Before merge, test on**:
- [x] Linux + Bundled GDB + Rust 1.46
- [x] Linux + Bundled LLDB + Rust 1.46
- [x] Linux + Bundled GDB + Rust 1.47
- [x] Linux + Bundled LLDB + Rust 1.47
- [x] macOS + Bundled LLDB + Rust 1.46
- [x] macOS + Bundled LLDB + Rust 1.47
- [ ] Windows + MinGW/Cygwin GDB + Rust 1.47
- [ ] Windows + MinGW/Cygwin GDB + Rust 1.46
**Does not work on Windows + MSVC LLDB** due to the lack of native Rust support patches

**After merge**
- [ ] Upstream to rustc

Co-authored-by: ortem <[email protected]>
Undin pushed a commit to intellij-rust/intellij-rust that referenced this pull request Oct 14, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 27, 2020
Use `clone_from` from `hashbrown::{HashMap,HashSet}`.

This change updates the `std` hash collections to use `hashbrown`'s `clone_from`, which was itself added in rust-lang#70052. Deriving `Clone` does not add a `clone_from` impl and uses the trait default, which calls `clone`.

Fixes rust-lang#28481
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clone_from in the standard library