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

Make SemanticsScope non-generic #5153

Merged
merged 1 commit into from
Jul 1, 2020
Merged

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jul 1, 2020

This slightly reduces the build times:

image

(compare to #1987 (comment))

@matklad
Copy link
Member

matklad commented Jul 1, 2020

Right, you are spot on @lnicola! I originally tried making Semantics non-generic, but didn't. I don't exactly remember why, it might be that either:

  • there's some annoying dyn limitation
  • I decided that sema.db being a concrete type would be useful

If it's the latter, we can have generic Semantics and concrete SemanticsImpl. I'll look into this if no one beats me to it.

@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020

I'm not sure if Semantics can be non-generic, it's a bit of an annoying change.

@matklad
Copy link
Member

matklad commented Jul 1, 2020

I'm not sure if Semantics can be non-generic,

The following setup should work without breaking the API:

pub struct Semantics<'db, DB>{
  db: &'db DB
  imp: SemanticsImpl<'a>
}

struct SemanticsImpl<'a> {
  db: &'db dyn HirDatabase
  s2d_cache: RefCell<SourceToDefCache>,
  cache: RefCell<FxHashMap<SyntaxNode, HirFileId>>,
}

@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020

What about the change in this PR? Do you think it's not a good idea?

@matklad
Copy link
Member

matklad commented Jul 1, 2020

Argh, I assumed this is an issue and not a PR. Gotta go make some pu erh I guess...

@matklad
Copy link
Member

matklad commented Jul 1, 2020

bors r+

This totally makes sense, but we need to do the rest as well!

@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020

One issue is that some places seem to require SourceDatabase instead of HirDatabase, so we might need to add more Upcast impls?

@bors
Copy link
Contributor

bors bot commented Jul 1, 2020

@bors bors bot merged commit 98ae447 into rust-lang:master Jul 1, 2020
@lnicola lnicola deleted the semantics-scope branch July 1, 2020 07:25
@matklad
Copy link
Member

matklad commented Jul 1, 2020

Adding upcasts should be fine!

@lnicola
Copy link
Member Author

lnicola commented Jul 1, 2020

Ugh, after a while ra_hir_def is back to 176s for me, so maybe this PR didn't do anything 😕.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants