Rename mir::Rvalue to Op. #70928
Rename mir::Rvalue to Op. #70928
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
cc @rust-lang/wg-mir-opt |
| rvalue: &mir::Rvalue<'tcx>, | ||
| rvalue: &mir::Op<'tcx>, |
There was a problem hiding this comment.
Probably the next thing to do here would be to rename rvalue variables.
| @@ -215,7 +215,7 @@ impl<'mir, 'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> Visitor<'tcx> | |||
| fn visit_assign( | |||
| &mut self, | |||
| place: &mir::Place<'tcx>, | |||
| rvalue: &mir::Rvalue<'tcx>, | |||
| rvalue: &mir::Op<'tcx>, | |||
| location: Location, | |||
| ) { | |||
| debug!("visit_assign(place={:?}, rvalue={:?})", place, rvalue); | |||
| @@ -2042,7 +2042,7 @@ impl<'tcx> Operand<'tcx> { | |||
| /// Rvalues | |||
There was a problem hiding this comment.
This documentation should also be adjusted
|
What does |
| } | ||
| } | ||
|
|
||
| /// If this rvalue supports a user-given type annotation, then | ||
| /// extract and return it. This represents the final type of the | ||
| /// rvalue and will be unified with the inferred type. | ||
| fn rvalue_user_ty(&self, rvalue: &Rvalue<'tcx>) -> Option<UserTypeAnnotationIndex> { | ||
| fn rvalue_user_ty(&self, rvalue: &Op<'tcx>) -> Option<UserTypeAnnotationIndex> { |
There was a problem hiding this comment.
also loads of function that should get renamed, not just variables
We could even rename |
|
@eddyb What's the motivation for this? |
| @@ -136,12 +136,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |||
| /// type writes its results directly into the memory specified by the place. | |||
| pub fn eval_rvalue_into_place( | |||
There was a problem hiding this comment.
another function name that needs a rename
|
I'm not sure about the rename, but I'm not against it either.
|
A long time ago, we got rid of "lvalue" everywhere and "rvalue" everywhere outside MIR. Anyway one of the reasons I told @bcata6 to open this before spending more time on it is so we can make sure it's not a dead-end (i.e. it's not a big deal if we just close this PR). |
|
Got it, thanks! |
|
Alright, I'm cc-ing the broader @rust-lang/compiler, and nominating this as a bikeshed item. |
eddyb
added
I-nominated
S-waiting-on-bikeshed
|
I think before merging this, we should also try to enumerate the other places that will need to be updated with this change and have a plan for updating those as well. For example,
|
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The reference and the UCG. I personally think "rvalue" is fairly established terminology by now, and |
It's no more established than it was at the time of e.g. rust-lang/rust-by-example#1028 (sadly, it looks like we didn't track this properly, which explains how we lost sight of |
|
Team member @pnkfelix has proposed to close this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rfcbot
added
proposed-final-comment-period
|
I think exactly this bike-shedding has happened before in wg-ucg, cc @RalfJung Personally i prefer something like |
|
|
I mostly have no idea about what's happening here, but I've checked my box because while name |
|
☔ The latest upstream changes (presumably #71079) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
Indeed, and
I like
Agreed. It is particularly confusing and inconsistent to have "rvalues" but not "lvalues", I think. |
I still think "operands" are what "operators"/"operations" operate on/with. Am I missing something? |
Well, yes, but that's not helpful here. By that standard The way I view it, the fundamental notions are places and values. This is reflected by the reference. Assignment puts a value into a place.
|
|
For what it's worth, I have found the term "place" to be "not great" when talking to people, particularly when it's referring (ultimately) to a place expression. I spend a while thinking about this in the polonius talk and I landed on "path expression" (or frequently just "path") as the term for "place expression", because it seemed like whenever I would try to explain what a "place" is, I always explained it by saying "it's a path, like a.b.c", and people understood that. In short, I think I largely agree with @RalfJung's proposed nomenclature of places and values as things that occur at runtime. I could live with place/value expressions as the thing we manipulate in our MIR programs (that will be evaluated to particular places/values), but actually I think the symmetry isn't that great, because it makes the terms rather verbose, and I think people won't want to use them. Personally, I prefer place/path -- i.e., what we call I was going to say I'd prefer value/expression, but I realize I can't really decide whether I think that expression should an Maybe we ought to try and draw up a big table with the options... or maybe introducing place/path is not helpful here. But it would be good to settle these names (and align them with UCG terminology). |
I feel rather strongly that it would be strange to call the dynamic concept a place and the static concept a path -- that sounds like they are unrelated. It should be "path expression" and "path value" or so.
Hm, I thought we had consensus for "value expression" in the UCG discussion, and that's also what the reference uses, for whatever that is worth. |
|
Heh, I'm probably just wrong -- or at least "unique" -- in finding four distinct, short words more memorable and evocative than compound terms like "place expression". I can certainly see that having the symmetry could help people trying to get their head around the distinctions at play. I still fear that the terms "place expression" and "value expression" are unwieldy and people will tend not to use them in full, but I will happily yield to the UCG consensus and stop trying to derail. =) But I do have a point of clarification...
But what does it use it to refer to -- what we call "rvalues" here or "operands"? Given your point (which I agree with) that the a-normalized form of MIR is a convenient implementation detail and not something "fundamental", I would expect that "value expressions" refers to "rvalues", and that operands are a kind of subset of that which we happen to find convenient (and hence they could, for example, be called "value subexpressions" or something like that, too). |
Yes that is my understanding, too. Hence the suggestion to rename "rvalue" to "value" in this PR. (The "expression" is implicit from being in the
Operands are a subset of values, but a "subexpression" is something else. I think "operand" is not bad, if I had to come up with a name I might have called it a "leaf value expression" or so (since operands are the leaves of value expressions). |
|
For the record, I'm fine with the names
That said, from that link you sent, @RalfJung:
In what way is an operand not itself a correct expression? I think that's what the |
|
Renaming "operand" to "subexpression" is not even "well-typed" in some ontological sense... there is no such thing as a "subexpression" in a global way, there are just "subexpressions of some given expression E". In other way, "subexpression" is a binary relation on expressions (it has type
Uh, it is, but so what? The defining property of an operand is not "this is a subexpression of some Renaming "operand" to "subexpression" is a bit like renaming "square" to "subshape" because, you know, every "square" is a "shape". |
|
Unnominating as it was discussed already in our last meeting. |
|
Renominated with the purpose of mentioning in the meeting that the discussion is still ongoing and maybe that the right place for the ongoing discussion is a specific Zulip topic. |
Dylan-DPC-zz
changed the title
|
Okay I'm officially moving the bikeshed regarding the name over to Zulip. Here is the Zulip thread for the bikeshed: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/To.20what.20to.20rename.20mir.3A.3ARvalue.20.2370928/near/195072985 I think we are all agreed that we won't land the renaming as encoded in this PR, so I am going to go ahead and close this PR. Thank you very much for the PR, @bcata6 ! |
rfcbot
removed
proposed-final-comment-period
|
So my understanding is that if a PR was opened that renamed |
Originally suggested by @eddyb.