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

[WIP] On-demandify region mapping #41327

Closed
wants to merge 10 commits into from

Conversation

cramertj
Copy link
Member

Attempt at #41057

This approach wound up introducing lots of churn, and I'm opening this primarily as an opportunity to discuss the change and see if there's a more principled way to break up region maps.

In particular, I'd like to see if there's a way to keep CodeExtents unique so that functions with access to only CodeExtents can lookup the associated function. Without this, I'm not sure of the right way to write things like fmt::Debug for CodeExtent (in src/librustc/middle/region.rs), or the cfg-generating (src/librustc/cfg/construct.rs) or error-reporting (src/librustc/infer/error_reporting/mod.rs), since they all try to access region maps using only a CodeExtent, and doing anything else would require a massive refactor (such as adding a NodeId to note_and_explain_region).

Thoughts?

r? @nikomatsakis

@cramertj
Copy link
Member Author

One possibility that occurred to me was to change CodeExtent from CodeExtent(u32) to CodeExtent(u32, DefId) where DefId is the ID of the top-level function with which it's associated. Thoughts?

@arielb1 arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 17, 2017
@nikomatsakis
Copy link
Contributor

@cramertj those are copied around quite frequently, and that would make the data structures much larger. I'll try to take tomorrow and form a more educated opinion. Thanks for the PR!

@cramertj
Copy link
Member Author

cramertj commented Apr 18, 2017

@nikomatsakis Yeah, that's why I didn't do it. :) I opened the PR hoping we could come to a better solution.

@bors
Copy link
Contributor

bors commented Apr 18, 2017

☔ The latest upstream changes (presumably #41357) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2017
@leonardo-m
Copy link

Can't you use something like this to reduce the amount of manual refactoring? http://coccinelle.lip6.fr/

@nikomatsakis
Copy link
Contributor

@leonardo-m plausibly, if it worked on rust, but I think that what @cramertj and I have been talking about is really more what we want the target code to look like, rather than the churn it takes to get there. That said, I've been doing a lot of refactoring on this PR on a side branch, and I think I'm going to close it for now (I've discussed with @cramertj off line), until I reach some point I am happy with.

@cramertj cramertj deleted the on-demandify-region-mapping branch September 21, 2017 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants