-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Enable cloning EntityHashMap and PreHashMap #11178
Enable cloning EntityHashMap and PreHashMap #11178
Conversation
It would probably be better to also swap the Clone impls on the generic parent structs to a manual clone impl: the lack of "perfect derives" means that it incorrectly requires a Clone bound. |
The parent struct ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though one open note about our use of compile tests.
use super::*; | ||
|
||
#[test] | ||
fn test_clone_entity_hash_map() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty partial to having something similar to static_assertions
(it's already in our dependency tree), which includes a assertion that will fail compilation if a type does not implement a trait. I think it should be fine to use it as a dev dependency over these tests which actually need to run when running our test suites.
This is a strict non-blocker and we can tackle this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah i was thinking of re-using trybuild
(https://docs.rs/trybuild/latest/trybuild/) since I see that it's already used in bevy somewhere else, but static_assertions seems to have more powerful checks
# Objective - We want to use `static_assertions` to perform precise compile time checks at testing time. In this PR, we add those checks to make sure that `EntityHashMap` and `PreHashMap` are `Clone` (and we replace the more clumsy previous tests) - Fixes #11181 (will need to be rebased once #11178 is merged) --------- Co-authored-by: Charles Bournhonesque <cbournhonesque@snapchat.com>
Objective
EntityHashMap
,EntityHashSet
andPreHashMap
are currently not Cloneable because of a missing trivialClone
bound forEntityHash
andPreHash
. This PR makes them Cloneable.(the parent struct
hashbrown::HashMap
requires theHashBuilder
to beClone
for theHashMap
to beClone
, see: https://github.com/rust-lang/hashbrown/blob/master/src/map.rs#L195)Solution
Clone
bound toPreHash
andEntityHash