-
Notifications
You must be signed in to change notification settings - Fork 155
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 IngredientIndex to KeyStruct #591
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
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.
Nice fix! Left some cleanup suggestions.
CodSpeed Performance ReportMerging #591 will not alter performanceComparing Summary
|
4bcbfee
to
47c318f
Compare
47c318f
to
acae02d
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.
Nice! I'm still toying with the name. Thoughts?
src/tracked_struct.rs
Outdated
/// allowing for multiple tracked structs with the same hash and ingredient_index | ||
/// created within the query to each have a unique id. | ||
#[derive(Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Copy, Clone)] | ||
pub struct DisambiguationKey { |
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.
It seems to me that the naming of DisambiguationKey
is not ideal, given e.g. KeyStruct
. I'd like some names that make the relationship clearer. Here are some possibilities -- hopefully it's clear which is meant to be which in each case!
HashStruct
andKeyStruct
(close to today)IdentityHash
andIdentity
IdentityHash
andDisambiguatedIdentityHash
IngredientAndHash
andDisambiguatorIngredientAndHash
(just name it after the contents)
What do you think @puuuuh?
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.
IdentityHash and Identity is the best variant, imo
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.
Also something like IdentityHash-StructCompoundId?
Closes #590