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

Visitor: remove context and add lifetimes. #17069

Merged
merged 2 commits into from
Sep 12, 2014
Merged

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Sep 7, 2014

Few visitors used the context passing feature and it can be easily emulated.
The added lifetime threading allows a visitor to keep safe references to AST
nodes it visits, making a non-owning ast_map design possible, for #13316.

@eddyb eddyb force-pushed the visitor branch 3 times, most recently from 780ac38 to 26322f6 Compare September 8, 2014 20:43
@emberian
Copy link
Member

emberian commented Sep 8, 2014

cc @pnkfelix

@eddyb eddyb force-pushed the visitor branch 2 times, most recently from 784d86d to 5d55f36 Compare September 8, 2014 22:00
@pnkfelix
Copy link
Member

pnkfelix commented Sep 9, 2014

The description above does not make it clear whether the presence of the context conflicted in some fundamental way with the lifetime-threading, or if it was just removed as drive-by code cleanup. (I tried to get clarification about this yesterday from @eddyb but I do not think he understood my question.

I do not mind the code cleanup; the fact that the context was so often () was a sign that the Visitor API had optimized for the wrong case. But I just want to make sure people reviewing the history get a clearer picture of why the change was made.

@eddyb
Copy link
Member Author

eddyb commented Sep 9, 2014

@pnkfelix My bad, I forgot to mention connection between the two changes, though I have to @nikomatsakis before starting work on this.

The context did not interfere with lifetime threading, but its removal makes the annotation overhead of lifetimes more bearable.
I thought that only adding syntax everywhere just to support ast-ptr was going to be viewed in a less positive light.

@eddyb
Copy link
Member Author

eddyb commented Sep 9, 2014

Turns out the cost is much smaller, there is no need to annotate methods which mention 'v only once, if you don't need to use the lifetime (like ast_map does in #13316).
I wouldn't have discovered that if I didn't notice that a newly added visit_pat method compiled after a rebase without me inserting a 'v.

@eddyb eddyb force-pushed the visitor branch 3 times, most recently from 1481aec to 29b44c6 Compare September 12, 2014 10:10
@pnkfelix
Copy link
Member

(I have to admit, the emulated contexts do not look as bad as I had feared, especially since @eddyb exhibited good taste in factoring the push/pop out into helper methods where appropriate.)

bors added a commit that referenced this pull request Sep 12, 2014
Few visitors used the context passing feature and it can be easily emulated.
The added lifetime threading allows a visitor to keep safe references to AST
nodes it visits, making a non-owning ast_map design possible, for #13316.
@bors bors closed this Sep 12, 2014
@bors bors merged commit 7ef6ff0 into rust-lang:master Sep 12, 2014
@eddyb eddyb deleted the visitor branch September 12, 2014 16:13
lnicola pushed a commit to lnicola/rust that referenced this pull request Apr 20, 2024
…icola

minor: Fix `rustc_skip_array_during_method_dispatch` edition check

CC rust-lang#16450
lnicola pushed a commit to lnicola/rust that referenced this pull request Apr 20, 2024
…icola

minor: Fix `rustc_skip_array_during_method_dispatch` edition check

CC rust-lang#16450
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.

4 participants