Skip to content

[MIR] Make things somewhat prettier #31616

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

Closed
wants to merge 1 commit into from

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Feb 12, 2016

After the MIR drop PR I was not quite satisfied with the free-standing functions in the mir::build::scope. They are free-standing purely to satisfy the borrowck. This PR satisfies borrowck in a different way by temporarily taking over the ownership of the scopes array (the with_scopes method). Everything else is pretty much unchanged, only moved around.

r? @nikomatsakis

@bors
Copy link
Collaborator

bors commented Feb 13, 2016

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

After the MIR drop PR I was not quite satisfied with the free-standing functions in the
mir::build::scope. They are free-standing purely to satisfy the borrowck. This PR satisfies
borrowck in a different way by temporarily taking over the ownership of the scopes array (the
with_scopes method). Everything else is pretty much unchanged, only moved around.
@nagisa nagisa force-pushed the mir-drop-terminator-take2 branch from 7a633ad to 8ce7e12 Compare February 16, 2016 20:28
@nikomatsakis
Copy link
Contributor

So, I personally prefer the free-standing functions. This way, I can read the code and I know that it doesn't use state in a way that would be invalid. Another option of course is to further modify the scopes vector to isolate the state better.

@nikomatsakis
Copy link
Contributor

Another option of course is to further modify the scopes vector to isolate the state better.

To clarify what I meant by this: the reason I initially introduced the cfg variable was precisely the same: so that I could add methods that would be callable even when some parts of the Builder were borrowed. Maybe we need/want a 'scopes' subset, not sure. But in general if subsetting (either via free fn or another type) is an option, I like it because otherwise I get worried we will recursively touch the scopes vector accidentally.

@nikomatsakis
Copy link
Contributor

Going to go ahead and close this PR then. @nagisa let me know if you want to discuss alternatives.

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.

3 participants