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

[NLL] make temp for each candidate in match arm #52733

Merged

Conversation

pnkfelix
Copy link
Member

In NLL, ref mut patterns leverage the two-phase borrow infrastructure to allow the shared borrows within a guard before the "activation" of the mutable borrow when we begin execution of the match arm's body. (There is further discussion of this on PR #50783.)

To accommodate the restrictions we impose on two-phase borrows (namely that there is a one-to-one mapping between each activation and the original initialization), this PR is making separate temps for each candidate pattern. So in an arm like this:

PatA(_, ref mut ident) | 
PatB(ref mut ident) | 
PatC(_, _, ref mut ident) | 
PatD(ref mut ident) if guard_stuff(ident) => ...

instead of 3 temps (two for the guard and one for the arm body), we now have 4 + 2 temps associated with ident: one for each candidate plus the actual temp that the guard uses directly, and then the sixth is the temp used in the arm body.

Fix #51348

pnkfelix added 3 commits July 26, 2018 13:15
… *for each* candidate pat.

This required a bit of plumbing to keep track of candidates. But I
took advantage of the hack session to try to improve the docs for the
relevant structs here.

(I also tried to simplify some of the related code in passing.)
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2018
@pnkfelix
Copy link
Member Author

@nikomatsakis you may want to skim over this in order to help decide whether we will try to resolve #51348 for EP2.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

just a few nits but looks good to me

{
let borrow_data = &mut self.idx_vec[borrow_index];
borrow_data.activation_location = TwoPhaseActivation::NotActivated;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

too bad we don't have NLL already! =)

@@ -126,7 +126,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
None,
remainder_span,
lint_level,
&pattern,
&[pattern.clone()],
Copy link
Contributor

Choose a reason for hiding this comment

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

is this just to convert from &T to &[T]? I know there was some helper for doing this somewhere...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, hey, the libs team finally gave in:

https://doc.rust-lang.org/std/slice/fn.from_ref.html

well, I guess it's still unstable.

Copy link
Member Author

Choose a reason for hiding this comment

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

i was lazy. will fix.

scope = this.declare_bindings(None, remainder_span, lint_level, &pattern,
ArmHasGuard(false), None);
scope = this.declare_bindings(
None, remainder_span, lint_level, &[pattern.clone()],
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, slice::from_ref?

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis -- @michaelwoerister feel free to take review back if you want to =)

@pnkfelix
Copy link
Member Author

I'm going to interpret niko's "looks good to me" as an r+.

@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 27, 2018

📌 Commit 9462645 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2018
@bors
Copy link
Contributor

bors commented Jul 27, 2018

⌛ Testing commit 9462645 with merge 6998b36...

bors added a commit that referenced this pull request Jul 27, 2018
…ate-in-arm, r=nikomatsakis

[NLL] make temp for each candidate in `match` arm

In NLL, `ref mut` patterns leverage the two-phase borrow infrastructure to allow the shared borrows within a guard before the "activation" of the mutable borrow when we begin execution of the match arm's body. (There is further discussion of this on PR #50783.)

To accommodate the restrictions we impose on two-phase borrows (namely that there is a one-to-one mapping between each activation and the original initialization), this PR is making separate temps for each candidate pattern. So in an arm like this:
```rust
PatA(_, ref mut ident) |
PatB(ref mut ident) |
PatC(_, _, ref mut ident) |
PatD(ref mut ident) if guard_stuff(ident) => ...
```

instead of 3 temps (two for the guard and one for the arm body), we now have 4 + 2 temps associated with `ident`: one for each candidate plus the actual temp that the guard uses directly, and then the sixth is the temp used in the arm body.

Fix #51348
@bors
Copy link
Contributor

bors commented Jul 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6998b36 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants