-
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
feature: add #[salsa::interned_sans_lifetime]
macro
#632
base: master
Are you sure you want to change the base?
feature: add #[salsa::interned_sans_lifetime]
macro
#632
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
CodSpeed Performance ReportMerging #632 will not alter performanceComparing Summary
|
@davidbarsky ....
|
We discussed that we want to discourage this pattern but it would also be nice if we can avoid duplicating the interned macro. Would it be possible to gate the functionality behind a feature? E.g. that the |
Given the goal is for this to only ease the transition for r-a it should be fine (and honestly preferred in my eyes) to gate this behind a feature until r-a no longer needs it (at which point we can remove it again). |
To expand as to why this is a separate macro instead of something like
|
It's hard to tell for me what the difference is between the two macros but would it be possible to change Or you might be able to do something similar to |
I do too, but difftastic helped here. I don't think it's a particularly useful paste, but if you have it handy,
I honestly recommend cloning this PR and diffing via difftastic.
Not that I'm aware of, at least with |
1612f16
to
6d23f3b
Compare
I fixed the rustdoc failures, but I can't reproduce the test failure w.r.t to cancellation on my Mac. That's a bit concerning, but I think I'll try and debug this tomorrow. |
(This consolidates #618, #620, and #623 into a single PR, logical PR.)
Here's what this solves:
'db
lifetime on interned structs makes migrating some rust-analyzer's existing interned values really challenging.We'll probably port rust-analyzer over the new, idiomatic API instead, but this makes the migration a lot easier!