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

Rename Update to StoreInSalsaDb or something like that #592

Open
nikomatsakis opened this issue Oct 13, 2024 · 7 comments
Open

Rename Update to StoreInSalsaDb or something like that #592

nikomatsakis opened this issue Oct 13, 2024 · 7 comments
Labels
bikeshed 🚴‍♀️ Debating API details and the like
Milestone

Comments

@nikomatsakis
Copy link
Member

The name Update is highly unintuitive. What this value really means is "safe to store in the salsa database".

@nikomatsakis nikomatsakis added the bikeshed 🚴‍♀️ Debating API details and the like label Oct 13, 2024
@nikomatsakis nikomatsakis added this to the Salsa 3.0 milestone Oct 13, 2024
@puuuuh
Copy link
Contributor

puuuuh commented Oct 21, 2024

SalsaIngredient?

@MichaReiser
Copy link
Contributor

I would prefer a term other than Ingredient because it's already a widely used term and I fear we would start overloading it.

I also think that Update is implemented by types that aren't ingredients, but are just part of Ingredients.

@nikomatsakis
Copy link
Member Author

Yeah, it is not an ingredient. It is for data structures that can be included in tracked/interned structs but which are not, themselves, tracked/interned structs.

@nikomatsakis
Copy link
Member Author

What about SalsaStructMember ?

@MichaReiser
Copy link
Contributor

My understanding of the Update trait is still very limited but I like the name if it is the trait that must be implemented by every salsa-struct field (maybe SalsaStructField?)

@nikomatsakis
Copy link
Member Author

I like the name SalsaStructField. Also, as discussed in our sync meeting today, I wonder if we should remove the 'static fallback (or, more likely, make it "opt-in" with an annotation on the field). The intent would be that deriving SalsaStructField is more efficient, and it also would make it possible for us to discourage people from using HashMap and HashSet (as opposed to FxHashSet etc) since those can easily lead to non-deterministic results.

@MichaReiser
Copy link
Contributor

The intent would be that deriving SalsaStructField is more efficient

I'm open to trying this on Ruff if we have a PR that adds this requirement to get some data on how painful it is for users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bikeshed 🚴‍♀️ Debating API details and the like
Projects
None yet
Development

No branches or pull requests

3 participants