-
Notifications
You must be signed in to change notification settings - Fork 176
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
Using std
maps in datagen
#2152
Conversation
455736e
to
c276f4e
Compare
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.
Not sure I like this. I migrated these from HashMap to LiteMap in #1341, and the reasons for doing it then haven't really changed.
#[allow(clippy::derive_hash_xor_eq)] | ||
|
||
// Custom implementation to avoid hashing the path. | ||
impl core::hash::Hash for ResourceKey { |
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.
Issue: I feel that ResourceKeyHash
should be used instead of ResourceKey
when a hash is needed, which is why there isn't already an impl Hash for ResourceKey
.
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.
But ResourceKeyHash
doesn't have the path, which I need for databake. This is like saying you should store f(x)
is a hash set instead of x
.
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 cannot approve this PR given that my PR #2115 conflicts with the Hash
impl. We can't merge both.
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.
What's the conflict exactly?
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.
We need to decide on what to do with the ResourceKeyMetadata.
- Does it affect PartialEq? (then it needs to go into Hash)
- Is it ignored for PartialEq? (then match statements get less efficient)
- Does it go into the path string itself? (then it gets into Hash and PartialEq automatically)
I don't want to lock ourselves into having a Hash impl if the impl would be inefficient.
tl;dr, let's reach consensus on #2115 and then we can move on.
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.
This is independent of #2115. Hashing on hash (aka path) only is valid, even if PartialEq compares more fields. Hash is free to produce collisions as long as it produces equal hashes for PartialEq values. Hash collisions don't affect correctness, they're a performance problem, but that doesn't matter here because a) it's only used in datagen and b) we don't have keys that differ only by metadata anyway.
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.
ok.
I think the reason was in part to get rid of Ord requirements |
One of the reasons you mention in #1341 was that we can add methods to The only valid point in #1341 imho is keeping |
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.
Once we decide on how to do metadata in ResourceKey, I'm okay merging this.
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.
.
Currently we're using
LiteMap
with no clear benefit but several drawbacks:HashMap
orBTreeMap
Ord
whereHash
would doHelloWorldDataProvider
can just use a static slice and doesn't have to allocate at all.