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

librustc: Parse, but do not fully turn on, the ref keyword for #15929

Merged
merged 1 commit into from
Aug 14, 2014

Conversation

pcwalton
Copy link
Contributor

by-reference upvars.

This partially implements RFC 38. A snapshot will be needed to turn this
on, because stage0 cannot yet parse the keyword.

Part of #12831.

r? @alexcrichton

@alexcrichton
Copy link
Member

I updated the PR message to point at #12831, feel free to update the commit message to point there as well (if you feel so inclined), but it's certainly ok regardless!

@nikomatsakis
Copy link
Contributor

so @aturon and I were talking and we were thinking that it is plausible that we could completely infer, on a variable by variable basis, both:

  1. Whether the variable ought to be captured "by value" or "by reference" (and, if by reference, what sort of reference) and
  2. Whether the closure itself should be &self, &mut self, or self.

In this case, we could do away entirely with the ref keyword and also with "self" indicators.

The idea is basically to make use of the same information we use today to decide what kind of borrow is needed. Basically after typeck (during regionck) we walk how each upvar is used. We can then say that every upvar which is either Copy or which is used by value must be taken by value, and the rest are by reference. Similarly, we can say that if you move out of something, it must be a once closure, and if you take an affine reference or mutate an upvar, it must be &mut self, but otherwise the closure can be &self (the most general case).

@nikomatsakis
Copy link
Contributor

(That's just an FYI, I'll try to write up something more detailed and mail it out)

@alexcrichton
Copy link
Member

Removing the r+ for now, @aturon recommended on IRC that we may want to hold off until we discuss this (not having ref would be nice!)

@pcwalton
Copy link
Contributor Author

Disagree. This RFC is approved, and therefore it should land. I do not want to hold off a critical piece of unboxed closures over a vague notion that we can do better. There are many things we will have to freeze in a suboptimal state to meet our 1.0 deadline. If we can't even land approved RFCs because of vague notions that we could do something better, we have a serious problem.

@huonw
Copy link
Member

huonw commented Jul 23, 2014

This can be approved to allow us to get some working form of unboxed closures, and then be removed and replaced with inference in future if we decide to go that route before 1.0.

@pcwalton
Copy link
Contributor Author

Agreed. Even with inference I think me may want a ref keyword so the programmer can choose to explicitly capture by reference if desired.

@alexcrichton
Copy link
Member

Ok, it's pretty squirreled away that we can at least land this part. One thing I forgot, can you add a test or two just to exercise some of this functionality? Other than that r=me

@nikomatsakis
Copy link
Contributor

On Wed, Jul 23, 2014 at 04:35:33PM -0700, Patrick Walton wrote:

Disagree.

Just to be clear, I specifically didn't say anything in my comment
about removing r+. I just wanted to get the idea out there, not hold
up progress.

@aturon
Copy link
Member

aturon commented Jul 24, 2014

This was my fault -- I just thought we might want to chat about it before driving ahead with the changes. But I think @pcwalton is right. Mea culpa.

by-reference upvars.

This partially implements RFC 38. A snapshot will be needed to turn this
on, because stage0 cannot yet parse the keyword.

Part of rust-lang#12381.
bors added a commit that referenced this pull request Aug 14, 2014
by-reference upvars.

This partially implements RFC 38. A snapshot will be needed to turn this
on, because stage0 cannot yet parse the keyword.

Part of #12831.

r? @alexcrichton
@bors bors closed this Aug 14, 2014
@bors bors merged commit a63003f into rust-lang:master Aug 14, 2014
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.

6 participants