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

Remove redundant CompileController entry points #34480

Merged
merged 2 commits into from
Jul 4, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Jun 26, 2016

Remove the after_expand and after_write_deps CompileController entry points.

The only things that separate these entry points from after_hir_lowering are dep-info generation and HIR map construction, neither of which is computationally intensive or has the potential to error.

r? @nrc

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 26, 2016

I could reintroduce the after_expand entry point earlier. I believe the most future compatible place would be after expansion and import resolution (since these will be merged soon) but before the rest of name resolution and HIR lowering (since these will ideally be merged at some point).

However, I'm not sure if having an entry point here would be useful enough to justify the added complexity -- @nrc what do you think?

@nrc
Copy link
Member

nrc commented Jun 27, 2016

I think it would be nice to have an entry point after expansion and before HIR lowering. I can imagine tools which want to operate on the expanded AST, but don't need the HIR or complete name resolution.

@jseyfried
Copy link
Contributor Author

@nrc Ok, I'll add after_expand between import resolution and the rest of name resolution.

@jseyfried
Copy link
Contributor Author

@nrc I rebased and added a commit that reintroduces after_expand between import resolution and the rest of name resolution.

@jseyfried jseyfried force-pushed the remove_entry_points branch 3 times, most recently from 7d86328 to 6c1b582 Compare June 30, 2016 16:49
@nrc
Copy link
Member

nrc commented Jun 30, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 30, 2016

📌 Commit 6c1b582 has been approved by nrc

@jseyfried
Copy link
Contributor Author

@bors r=nrc (fixed the commit message)

@bors
Copy link
Contributor

bors commented Jul 1, 2016

📌 Commit d1e3d62 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jul 1, 2016

⌛ Testing commit d1e3d62 with merge f29f8c2...

@bors
Copy link
Contributor

bors commented Jul 1, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Jul 1, 2016 at 5:34 AM, bors notifications@github.com wrote:

💔 Test failed - auto-win-msvc-64-opt-rustbuild
https://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/1625


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#34480 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAD95JGFxzbzuiSylzxJdOeip1yJiZ6tks5qRQk9gaJpZM4I-fbG
.

@bors
Copy link
Contributor

bors commented Jul 1, 2016

⌛ Testing commit d1e3d62 with merge 2ce10f5...

@bors
Copy link
Contributor

bors commented Jul 1, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@jseyfried
Copy link
Contributor Author

@bors retry

@bors
Copy link
Contributor

bors commented Jul 4, 2016

⌛ Testing commit d1e3d62 with merge bbdbe99...

bors added a commit that referenced this pull request Jul 4, 2016
Remove redundant `CompileController` entry points

Remove the `after_expand` and `after_write_deps` `CompileController` entry points.

The only things that separate these entry points from `after_hir_lowering` are dep-info generation and HIR map construction, neither of which is computationally intensive or has the potential to error.

r? @nrc
@bors bors merged commit d1e3d62 into rust-lang:master Jul 4, 2016
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