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.

Sign up for GitHub

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

Jump to bottom

More thread-safety changes #49390

Merged
merged 10 commits into from
Apr 10, 2018
Merged

More thread-safety changes #49390

merged 10 commits into from
Apr 10, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 26, 2018

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2018
@michaelwoerister
Copy link
Member

@rust-lang/compiler Does anybody know what LazyTokenStream is? It seems to be unused.

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 27, 2018 •
edited
Loading

@michaelwoerister

Does anybody know what LazyTokenStream is?

It's a hack! But it's used.
Many nonterminal tokens (e.g. NtExpr) don't know their original token streams from which they were parsed (they have only AST fragments like ast::Expr), but these token streams are sometimes needed (for example if they are a part of macro definition and that macro definition needs to be stored in metadata), so AST fragments are pretty-printed into text (!) and then parsed back into the token stream (on best-effort basis).
LazyTokenStream serves as a cache for these AST -> TokenStream conversions to avoid doing these conversions multiple times, so removing it won't affect correctness, only performance.

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 28, 2018

@petrochenkov Why do we keep the AST around in the token stream though? It seems more correct to use the original TokenStream instead of relying on an lossy AST -> TokenStream transformation

@petrochenkov
Copy link
Contributor

@Zoxc

Why do we keep the AST around in the token stream though?

Who knows. Nonterminal is represented with AST fragments since at least 2012, years before token streams were introduced.
Someone needs to port expansion code working with nonterminals to token trees.

@shepmaster
Copy link
Member

Ping from triage, @michaelwoerister !

michaelwoerister
michaelwoerister reviewed Apr 9, 2018
View reviewed changes
src/librustc/hir/map/mod.rs
@@ -927,8 +928,13 @@ impl<'hir> Map<'hir> {
}

pub fn intern_inlined_body(&self, def_id: DefId, body: Body) -> &'hir Body {
let mut inlined_bodies = self.inlined_bodies.borrow_mut();
if let Some(&b) = inlined_bodies.get(&def_id) {
debug_assert_eq!(&body, b);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this condition hold? body is on the stack and b is in the arena. Am I overlooking something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a pointer comparison. This check ensures that we always give the same body for the same def_id to intern_inlined_body.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right of course :)

@michaelwoerister
Copy link
Member

@Zoxc, looks good to me except for intern_inlined_body. I think you can just remove the assertion there.

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2018

📌 Commit 1e3f638 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2018
@bors
Copy link
Contributor

bors commented Apr 10, 2018

⌛ Testing commit 1e3f638 with merge 67712d7...

bors added a commit that referenced this pull request Apr 10, 2018
@bors
Auto merge of #49390 - Zoxc:sync-syntax, r=michaelwoerister
67712d7 
More thread-safety changes

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Apr 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 67712d7 to master...

@bors bors merged commit 1e3f638 into rust-lang:master Apr 10, 2018
@bors bors mentioned this pull request Apr 10, 2018
Closed
This was referenced Apr 10, 2018
Merged
Closed
Merged
Merged
Merged
@Zoxc Zoxc deleted the sync-syntax branch April 10, 2018 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Zoxc @michaelwoerister @petrochenkov @shepmaster @bors @pietroalbini