Add 'db lifetime to BuiltinDeriveMacroId - #23060
Conversation
rustbot
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
3299abc to
43c5b6d
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
913ef11 to
85dbb37
Compare
- use `from_id!` for `EnumVariant{,Id}`
- use `?` for consistency with the other methods
- use `LazyCell` for lazy initialization
- rm needlessly complicated `Ordering` clause
- use chained if-lets to reduce nesting
- rm manual lifetime expansion (`parent_arc` seems to have been an Arc
in the past)
- use `.into()` to turn things into `Definition`s
A lot of these were necessary to allow the next commit to compile, but a few are miscellaneous.
85dbb37 to
d3f8893
Compare
d3f8893 to
d5387cb
Compare
There was a problem hiding this comment.
I'm a bit hesitant to merge this while @dfireBird has its lifetimes PR in flight; this PR is much easier to review but the work there is definitely higher...
| ) { | ||
| // Lazily initialised when we first encounter a `#[doc = macro!()]`. | ||
| let mut expander: Option<DocMacroExpander<'db>> = None; | ||
| let mut expander = LazyCell::new(|| { |
There was a problem hiding this comment.
Why this change? (LazyCell has an additional branch).
There was a problem hiding this comment.
Mostly code clarity I'd say? LazyCell makes it very clear what's happening: a value that is initialized lazily.
LazyCellhas an additional branch
You mean to check whether the value has been initialized or not? But I think Option::get_or_insert_with does a similar check for None, no?
Anyway I don't feel too strongly about this, happy to revert.
There was a problem hiding this comment.
LazyCell needs to check for reentrance.
I wouldn't oppose if this was written this way to begin with, but I do oppose changing it in an unrelated PR.
| fn adjust_to_nearest_non_block_module<'db, 'dm>( | ||
| db: &'db dyn SourceDatabase, | ||
| mut def_map: &'db DefMap, | ||
| mut def_map: &'dm DefMap<'db>, |
There was a problem hiding this comment.
I think you can just use the 'db lifetime here, after all the DefMap is stored in the db.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Why this test? Is it for the DynMap? Most of it is type gymnastics, I don't think it needs tests.
There was a problem hiding this comment.
It was a leftover from Lukas' code snippet – I'll remove it if you say it's unnecessary
| } | ||
|
|
||
| #[salsa::interned(debug, unsafe(no_lifetime), revisions = usize::MAX)] | ||
| #[salsa::interned(debug)] |
There was a problem hiding this comment.
I wouldn't remove revisions = usize::MAX yet. We need to take a conscious decision to enable interneds GC and in what frequency.
| lang_items: &'db LangItems, | ||
| resolver: &'a Resolver<'db>, | ||
| store: &'db ExpressionStore, | ||
| store: &'a ExpressionStore, |
There was a problem hiding this comment.
Why this change? It should remain 'db.
|
Happy to wait for @dfireBird's PR to land first – I honestly doubted I would be able to drive this to completion, but since I now have, fixing it up as needed (by reading) sounds doable |
3 participants
Part of #22868