-
Notifications
You must be signed in to change notification settings - Fork 192
support interned enums #1027
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
support interned enums #1027
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #1027 will improve performances by 5.73%Comparing Summary
Benchmarks breakdown
|
90f6c8c to
7564e5b
Compare
7564e5b to
80cae02
Compare
MichaReiser
left a comment
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.
Thank you for working on this.
| Ok(crate::debug::dump_tokens( | ||
| struct_ident, | ||
| quote! { | ||
| #(#additional_items)* |
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 not a 100% sure whether this is possible but I think it should be.
Could we move the synthesized struct definition within the const _: () => { block. It has the advantage that we can auto generate a name for the interned 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.
Can you add or extend an existing persistence test to show that persistence works with interned enums too
MichaReiser
left a comment
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.
Hmm, thinking more about this, I don't think this will work, or at least, would be very confusing.
Up to now, a one-argument query can take any salsa-struct (interned, input, or tracked). With this change, this would no longer be the case for interned enums because they don't implement AsId (they can't, they don't know the ID).
I'll close this PR (and the issue) for now as I'm no longer convinced we want this feature. Thanks for giving it a try, it helped me understand why this isn't what we need
close #1026