-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
rustc: rename all occurences of "freevar" to "upvar". #60530
Conversation
This comment has been minimized.
This comment has been minimized.
Hmm. I don't know where the terminology 'upvar' came from -- I had never heard it before the rust project, but I also remember it's meaning seemed sort of intuitive and obvious, and I have kind of adopted it. "Free variables" is definitely the more standard term for PL folks, but I think not well known outside PL research circles. When explaining the code, I feel like I always have the need to define upvar, and I do tend to say "capture", so I think I agree that this is the word we should move to in the future -- especially since captures (per RFC rust-lang/rfcs#2229) will correspond to captured paths and not variables in the future. Anyway, 👍 to this PR for moving in a good, consistent direction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with rebase
/// Closure bound must not outlive captured free variables | ||
FreeVariable(Span, ast::NodeId), | ||
/// Closure bound must not outlive captured variables | ||
ClosureCapture(Span, ast::NodeId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight inconsistency that this is changed to Capture
directly, but if we're moving there anyway (which I support), then ok by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did this because the comment and the user-facing message this causes both contain the word "capture".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UpvarCapture
(as in, "the capture of an upvar") would've also probably worked, but this seemed nicer.
☔ The latest upstream changes (presumably #60544) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors r=oli-obk |
📌 Commit 8d9f4a1 has been approved by |
rustc: rename all occurences of "freevar" to "upvar". Most of the more recent code talks about "(closure) upvars", so I believe that's the name we want to use. There's also the possibility of using "capture" which is more user-facing, but I'd rather not change *both* "freevar" and "upvar" to something else in this one PR. cc @nikomatsakis @petrochenkov
Rollup of 5 pull requests Successful merges: - #60131 (Fix broken link in rustc_plugin doc) - #60426 (Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting) - #60515 (use span instead of div for since version) - #60530 (rustc: rename all occurences of "freevar" to "upvar".) - #60536 (Correct code points to match their textual description) Failed merges: r? @ghost
Most of the more recent code talks about "(closure) upvars", so I believe that's the name we want to use.
There's also the possibility of using "capture" which is more user-facing, but I'd rather not change both "freevar" and "upvar" to something else in this one PR.
cc @nikomatsakis @petrochenkov