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

Disowned the Visitor. #11161

Merged
merged 1 commit into from
Jan 6, 2014
Merged

Disowned the Visitor. #11161

merged 1 commit into from
Jan 6, 2014

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Dec 27, 2013

The primary user of @T/P<T> references from Visitor was ast_map, which in turn had two users (phase 3 in rustc and a step in loading items from metadata).
Both of them have been rewritten to use ast_map as a folder (this might speed up the compilation time of stage2 rustc by 100-200ms just because a fold + a visit are merged together).

@eddyb
Copy link
Member Author

eddyb commented Dec 27, 2013

I've missed the second user of @T - borrowck - which used to store @Exprs and @Pats in move_data::MoveKind.
I've attempted giving Visitor an explicit lifetime (which could propagate downwards - the lifetime of every reference is the lifetime of the crate you're vising), but even without considering the refactoring implications, I've hit an ICE cold dead end - see #5121.

cc @pcwalton this might interfere with your plans to remove @mut from libsyntax.

@pcwalton
Copy link
Contributor

It's probably easier to just change the borrow checker to not store reference counted expressions in it.

@alexcrichton
Copy link
Member

Closing for now. Sadly we have a very large queue, and I'd like to keep the queue clear of "work in progress" patches that aren't necessarily ready for merging yet.

This is some awesome work though, and thanks!

@eddyb
Copy link
Member Author

eddyb commented Dec 30, 2013

@pcwalton turns out the node ID is already there, the @Exprs and @Pats aren't required at all (and looking up the ID is only needed for error reporting).

@emberian emberian reopened this Jan 4, 2014
@@ -592,7 +589,9 @@ pub fn pretty_print_input(sess: Session,
} as @pprust::pp_ann
}
PpmTyped => {
let analysis = phase_3_run_analysis_passes(sess, &crate);
// Should be always Some for ppm == PpmTyped.
let ast_map = ast_map.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

change to expect with a msg, to make the eventual ICE less unpleasant

@eddyb
Copy link
Member Author

eddyb commented Jan 5, 2014

That is curious. I didn't realize ast::Pat are not truly part of the AST map, only node_locals which come from PatIdent (and contain only the Ident) or self.
Because of self being a special case, the best I can do is add an Option<@Pat> in node_local (won't make ast_node larger) and use that when a span is required.

bors added a commit that referenced this pull request Jan 6, 2014
The primary user of `@T`/`P<T>` references from `Visitor` was `ast_map`, which in turn had two users (phase 3 in rustc and a step in loading items from metadata).
Both of them have been rewritten to use `ast_map` as a folder (this might speed up the compilation time of stage2 rustc by 100-200ms just because a fold + a visit are merged together).
@bors bors closed this Jan 6, 2014
@bors bors merged commit 3119d18 into rust-lang:master Jan 6, 2014
@eddyb eddyb deleted the de-at-visitor branch January 6, 2014 13:54
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 31, 2023
[significant_drop_tightening] Fix rust-lang#11160

Fix rust-lang#11160

```
changelog: [`significant_drop_tightening`]: Ignore literals in function returns
```
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.

7 participants