This is an archive of the discontinued LLVM Phabricator instance.

[clang] NFC: Rename rvalue to prvalue
ClosedPublic

Authored by mizvekov on Jun 4 2021, 2:40 PM.

Details

Summary

This renames the expression value categories from rvalue to prvalue,
keeping nomenclature consistent with C++11 onwards.

C++ has the most complicated taxonomy here, and every other language
only uses a subset of it, so it's less confusing to use the C++ names
consistently, and mentally remap to the C names when working on that
context (prvalue -> rvalue, no xvalues, etc).

Renames:

  • VK_RValue -> VK_PRValue
  • Expr::isRValue -> Expr::isPRValue
  • SK_QualificationConversionRValue -> SK_QualificationConversionPRValue
  • JSON AST Dumper Expression nodes value category: "rvalue" -> "prvalue"

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Jun 4 2021, 2:40 PM
mizvekov requested review of this revision.Jun 4 2021, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 2:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov updated this revision to Diff 349987.Jun 4 2021, 4:09 PM

rebase
some small comment changes
apply format patch to tests

Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 4:09 PM
mizvekov edited the summary of this revision. (Show Details)Jun 5 2021, 8:54 AM
mizvekov added reviewers: rsmith, aaronpuchert.

Thank you for the terminology cleanup! I'm not opposed to the changes, but I am wondering whether the churn is worth it given that the nomenclature will still be confusing for those thinking about non-C++ based languages (esp if someone in the future wants to add Expr::isRValue() with the C++ meaning of prvalue || xvalue). Either way, I think the confusion will likely be relatively minor and temporary.

@rsmith, what say you?

(esp if someone in the future wants to add Expr::isRValue() with the C++ meaning of prvalue || xvalue).

@rsmith actually suggested we add isRValue as a second step here, and I think it's a good idea, but I was also
worried a bit about the practical issue:
Others rebasing patches on top of that change without realizing it, and introducing possibly subtle issues :)
We could wait just a little bit for that and let others just soak in the build breakage meanwhile.

mizvekov edited the summary of this revision. (Show Details)Jun 8 2021, 12:38 PM
rsmith added a comment.Jun 8 2021, 2:54 PM

I think this is worth doing -- "rvalue" is at least ambiguous and, in C++-specific cases, confusing and wrong. Saying "prvalue" in C-specific parts of clang may also be a bit surprising, but it's unambiguous and still meaningful.

I think we shouldn't consider adding an isRValue until the dust has settled and we're used to using "prvalue" for prvalues. Even then, we should be cautious about it, because people might reach for it when they mean prvalue (and in any case I'd expect isRValue() checks to be quite rare.

clang/include/clang/AST/Expr.h
266–271

This comment needs an update.

clang/lib/AST/ExprConstant.cpp
2508

I'd leave the assert message alone here: for (better or) worse, C++ calls this conversion an "lvalue-to-rvalue conversion" even though it converts glvalues to prvalues.

clang/lib/Sema/Sema.cpp
583
597
clang/lib/Sema/SemaExprCXX.cpp
5662

Hm, I wonder if this it's correct that these both evaluate to false for an xvalue. Does anyone have a copy of Embarcadero's 32-bit compiler to test with?

clang/lib/Sema/SemaInit.cpp
3588

Would be nice to rename this SK_ enumerator as a follow-up.

mizvekov updated this revision to Diff 350729.Jun 8 2021, 3:51 PM

Implement rsmith's suggestions.

mizvekov marked 4 inline comments as done.Jun 8 2021, 3:53 PM
mizvekov marked an inline comment as done.Jun 8 2021, 4:09 PM
mizvekov added inline comments.
clang/lib/Sema/SemaInit.cpp
3588

Nice catch! Missed this one...

Done as a follow-up patch at: D103933

mizvekov marked an inline comment as done.Jun 8 2021, 4:38 PM
mizvekov added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
5662

I tested, looks correct:

#include <cstdio>

#define TEST(E) printf(#E " rvalue:%d lvalue:%d\n", __is_rvalue_expr(E), __is_lvalue_expr(E))

int main() {
  TEST(1);

  int x;
  TEST(x);
  TEST(static_cast<int&&>(x));

  struct T { int m; };
  TEST(T{}.m);

  return 0;
}
bcc32c test.cpp
.\test.exe
1 rvalue:1 lvalue:0
x rvalue:0 lvalue:1
static_cast<int&&>(x) rvalue:0 lvalue:0
T{}.m rvalue:0 lvalue:0
rsmith accepted this revision.Jun 8 2021, 6:20 PM

Looks good to me.

This revision is now accepted and ready to land.Jun 8 2021, 6:20 PM
This revision was automatically updated to reflect the committed changes.