Skip to content

Conversation

pnkfelix
Copy link
Contributor

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
Contributor

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
Contributor 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
Contributor 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
Contributor Author

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

@pnkfelix
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Collaborator

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
Collaborator

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
Collaborator

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