[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4819b751bd87: [clang] NFC: change uses of 
`Expr->getValueKind` into `is?Value` (authored by mizvekov).

Changed prior to commit:
  https://reviews.llvm.org/D100733?vs=351123&id=362254#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp

Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5832,7 +5832,7 @@
  Entity.getType()) &&
 canPerformArrayCopy(Entity)) {
   // If source is a prvalue, use it directly.
-  if (Initializer->getValueKind() == VK_PRValue) {
+  if (Initializer->isPRValue()) {
 AddArrayInitStep(DestType, /*IsGNUExtension*/false);
 return;
   }
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -6897,7 +6897,7 @@
   assert(!isa(E) && "Double-bound temporary?");
 
   // If the result is a glvalue, we shouldn't bind it.
-  if (!E->isPRValue())
+  if (E->isGLValue())
 return E;
 
   // In ARC, calls that return a retainable type can return retained,
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5546,7 +5546,7 @@
 BaseExpr = LHSExp;// vectors: V[123]
 IndexExpr = RHSExp;
 // We apply C++ DR1213 to vector subscripting too.
-if (getLangOpts().CPlusPlus11 && LHSExp->getValueKind() == VK_PRValue) {
+if (getLangOpts().CPlusPlus11 && LHSExp->isPRValue()) {
   ExprResult Materialized = TemporaryMaterializationConversion(LHSExp);
   if (Materialized.isInvalid())
 return ExprError();
@@ -10159,7 +10159,7 @@
 RHSType, DiagID))
 return RHSType;
 } else {
-  if (LHS.get()->getValueKind() == VK_LValue ||
+  if (LHS.get()->isLValue() ||
   !tryGCCVectorConvertAndSplat(*this, &LHS, &RHS))
 return RHSType;
 }
@@ -14939,7 +14939,7 @@
 // complex l-values to ordinary l-values and all other values to r-values.
 if (Input.isInvalid()) return ExprError();
 if (Opc == UO_Real || Input.get()->getType()->isAnyComplexType()) {
-  if (Input.get()->getValueKind() != VK_PRValue &&
+  if (Input.get()->isGLValue() &&
   Input.get()->getObjectKind() == OK_Ordinary)
 VK = Input.get()->getValueKind();
 } else if (!getLangOpts().CPlusPlus) {
@@ -19176,7 +19176,7 @@
   Expr *SubExpr = SubResult.get();
   E->setSubExpr(SubExpr);
   E->setType(S.Context.getPointerType(SubExpr->getType()));
-  assert(E->getValueKind() == VK_PRValue);
+  assert(E->isPRValue());
   assert(E->getObjectKind() == OK_Ordinary);
   return E;
 }
@@ -19186,7 +19186,7 @@
 
   E->setType(VD->getType());
 
-  assert(E->getValueKind() == VK_PRValue);
+  assert(E->isPRValue());
   if (S.getLangOpts().CPlusPlus &&
   !(isa(VD) &&
 cast(VD)->isInstance()))
@@ -19277,7 +19277,7 @@
 return ExprError();
   }
 
-  assert(E->getValueKind() == VK_PRValue);
+  assert(E->isPRValue());
   assert(E->getObjectKind() == OK_Ordinary);
   E->setType(DestType);
 
@@ -19437,7 +19437,7 @@
 ExprResult RebuildUnknownAnyExpr::VisitImplicitCastExpr(ImplicitCastExpr *E) {
   // The only case we should ever see here is a function-to-pointer decay.
   if (E->getCastKind() == CK_FunctionToPointerDecay) {
-assert(E->getValueKind() == VK_PRValue);
+assert(E->isPRValue());
 assert(E->getObjectKind() == OK_Ordinary);
 
 E->setType(DestType);
@@ -19451,7 +19451,7 @@
 E->setSubExpr(Result.get());
 return E;
   } else if (E->getCastKind() == CK_LValueToRValue) {
-assert(E->getValueKind() == VK_PRValue);
+assert(E->isPRValue());
 assert(E->getObjectKind() == OK_Ordinary);
 
 assert(isa(E->getType()));
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -877,7 +877,7 @@
 
   // If the expression is a temporary, materialize it as an lvalue so that we
   // can use it multiple times.
-  if (E->getValueKind() == VK_PRValue)
+  if (E->isPRValue())
 E = CreateMaterializeTemporaryExpr(E->getType(), E, true);
 
   // The location of 

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Also by the way D100713  is not really a 
dependency here, this DR can land on it's own if that is not clear.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 351123.
mizvekov added a comment.

Rebase.

Now that we have isPRValue, this is hopefully less controversial now :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp

Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5832,7 +5832,7 @@
  Entity.getType()) &&
 canPerformArrayCopy(Entity)) {
   // If source is a prvalue, use it directly.
-  if (Initializer->getValueKind() == VK_PRValue) {
+  if (Initializer->isPRValue()) {
 AddArrayInitStep(DestType, /*IsGNUExtension*/false);
 return;
   }
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -6870,7 +6870,7 @@
   assert(!isa(E) && "Double-bound temporary?");
 
   // If the result is a glvalue, we shouldn't bind it.
-  if (!E->isPRValue())
+  if (E->isGLValue())
 return E;
 
   // In ARC, calls that return a retainable type can return retained,
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5541,7 +5541,7 @@
 BaseExpr = LHSExp;// vectors: V[123]
 IndexExpr = RHSExp;
 // We apply C++ DR1213 to vector subscripting too.
-if (getLangOpts().CPlusPlus11 && LHSExp->getValueKind() == VK_PRValue) {
+if (getLangOpts().CPlusPlus11 && LHSExp->isPRValue()) {
   ExprResult Materialized = TemporaryMaterializationConversion(LHSExp);
   if (Materialized.isInvalid())
 return ExprError();
@@ -10129,7 +10129,7 @@
 RHSType, DiagID))
 return RHSType;
 } else {
-  if (LHS.get()->getValueKind() == VK_LValue ||
+  if (LHS.get()->isLValue() ||
   !tryGCCVectorConvertAndSplat(*this, &LHS, &RHS))
 return RHSType;
 }
@@ -14852,7 +14852,7 @@
 // complex l-values to ordinary l-values and all other values to r-values.
 if (Input.isInvalid()) return ExprError();
 if (Opc == UO_Real || Input.get()->getType()->isAnyComplexType()) {
-  if (Input.get()->getValueKind() != VK_PRValue &&
+  if (Input.get()->isGLValue() &&
   Input.get()->getObjectKind() == OK_Ordinary)
 VK = Input.get()->getValueKind();
 } else if (!getLangOpts().CPlusPlus) {
@@ -19049,7 +19049,7 @@
   Expr *SubExpr = SubResult.get();
   E->setSubExpr(SubExpr);
   E->setType(S.Context.getPointerType(SubExpr->getType()));
-  assert(E->getValueKind() == VK_PRValue);
+  assert(E->isPRValue());
   assert(E->getObjectKind() == OK_Ordinary);
   return E;
 }
@@ -19059,7 +19059,7 @@
 
   E->setType(VD->getType());
 
-  assert(E->getValueKind() == VK_PRValue);
+  assert(E->isPRValue());
   if (S.getLangOpts().CPlusPlus &&
   !(isa(VD) &&
 cast(VD)->isInstance()))
@@ -19150,7 +19150,7 @@
 return ExprError();
   }
 
-  assert(E->getValueKind() == VK_PRValue);
+  assert(E->isPRValue());
   assert(E->getObjectKind() == OK_Ordinary);
   E->setType(DestType);
 
@@ -19304,7 +19304,7 @@
 ExprResult RebuildUnknownAnyExpr::VisitImplicitCastExpr(ImplicitCastExpr *E) {
   // The only case we should ever see here is a function-to-pointer decay.
   if (E->getCastKind() == CK_FunctionToPointerDecay) {
-assert(E->getValueKind() == VK_PRValue);
+assert(E->isPRValue());
 assert(E->getObjectKind() == OK_Ordinary);
 
 E->setType(DestType);
@@ -19318,7 +19318,7 @@
 E->setSubExpr(Result.get());
 return E;
   } else if (E->getCastKind() == CK_LValueToRValue) {
-assert(E->getValueKind() == VK_PRValue);
+assert(E->isPRValue());
 assert(E->getObjectKind() == OK_Ordinary);
 
 assert(isa(E->getType()));
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -897,7 +897,7 @@
 
   // If the expression is a temporary, materialize it as an lvalue so that we
   // can use it multiple times.
-  if (E->getValueKind() == VK_PRValue)
+  if (E->isPRValue())
 E = CreateMaterializeTemporaryExpr(E->getType(), E, true);
 
   // The location of the `co_await` token cannot be used when constructing
@@ -957,7 +957,7 @@
 
   // If the expression is a temporary, materializ

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

I created a DR which proposes the renaming as rsmith suggested: 
https://reviews.llvm.org/D103720

In D100733#2773944 , @aaronpuchert 
wrote:

> A new value category feels like a global change for a local problem. We can 
> explain the behavior that we want without introducing a new value category: 
> either as a “combined” overload resolution for xvalue and lvalue, where all 
> candidates for the xvalue are strictly better than any candidate for the 
> lvalue, or as a two-phase resolution that falls back only if there are no 
> candidates. These explanations are equivalent.

Sure, I had it in mind that is not necessarily how that would be explained to 
the users, but perhaps as the standard text is more geared towards the 
toolchain folks, the original explanation might be more suited there, and this 
alternative one is more didactic, as you pointed out it can be explained just 
in the context of implicit moves.

> My impression is that some people didn't like the changes that came with 
> C++11 and that it made the terminology of C and C++ somewhat inconsistent. 
> (Though I think you can work with the C++ terminology in C, there are just no 
> xvalues, but you can't work the other way around.)

Thanks. That makes sense!

In D100733#2773944 , @aaronpuchert 
wrote:

> But perhaps I would not introduce for now an `isRValue` function. (Although C 
> doesn't have xvalues, so maybe it doesn't matter?)



In D100733#2761031 , @rsmith wrote:

> What do we think about renaming `isRValue()` to `isPRValue()` and renaming 
> `VK_RValue` to `VK_PRValue`, adding a "real" `isRValue()`, and then 
> performing this cleanup?

We might want to hold on to the second step there (adding back another 
isRValue) for a little while, to give time for people to rebase patches and 
such, so they get a friendlier build breakage instead of some more subtle issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D100733#2760890 , @mizvekov wrote:

> - I still think a new value category is simpler than the two phase overload 
> resolution it replaces :D

Not sure if it's simpler, but it'll produce less confusing results.

> - This new value category would (so far as this is only an experiment I am 
> running), only apply to standards older than c++23, where from then on it's 
> just the current categories and the rules as in P2266 
> .
> - Even in the old standards it replaces, it does not introduce any drastic 
> changes. This new category is the same as XValues as far as overload 
> resolution is concerned (with the small addition that it binds less 
> preferentially to non-const l-value references), and as far as type deduction 
> goes, it behaves exactly as LValues.

A new value category feels like a global change for a local problem. We can 
explain the behavior that we want without introducing a new value category: 
either as a “combined” overload resolution for xvalue and lvalue, where all 
candidates for the xvalue are strictly better than any candidate for the 
lvalue, or as a two-phase resolution that falls back only if there are no 
candidates. These explanations are equivalent.

In D100733#2761031 , @rsmith wrote:

> What do we think about renaming `isRValue()` to `isPRValue()` and renaming 
> `VK_RValue` to `VK_PRValue`, adding a "real" `isRValue()`, and then 
> performing this cleanup?

One problem might be that C and C++ have different terminology, right? I like 
the idea of going with prvalue instead of rvalue, then it's the right thing for 
C++ and if you're coming from the C world it's at least not confusing. But 
perhaps I would not introduce for now an `isRValue` function. (Although C 
doesn't have xvalues, so maybe it doesn't matter?)

In D100733#2761052 , @mizvekov wrote:

> Is there any special reason we ended up in this state of affairs with the 
> swapped out names?

My impression is that some people didn't like the changes that came with C++11 
and that it made the terminology of C and C++ somewhat inconsistent. (Though I 
think you can work with the C++ terminology in C, there are just no xvalues, 
but you can't work the other way around.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D100733#2761031 , @rsmith wrote:

> What do we think about renaming `isRValue()` to `isPRValue()` and renaming 
> `VK_RValue` to `VK_PRValue`, adding a "real" `isRValue()`, and then 
> performing this cleanup? I think the current state of treating "rvalue" as 
> sometimes meaning rvalue and sometimes meaning "prvalue" is unhelpful and 
> confusing, and this change makes it worse because `isRValue` *could* be 
> checking for an rvalue whereas a comparison against `VK_RValue` more clearly 
> is only looking for one specific value category rather than two.

Sounds good. For some reason I thought this would be more controversial...

Is there any special reason we ended up in this state of affairs with the 
swapped out names?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

I don't think out-of-tree experiments on possibilities for move semantics are 
especially motivating for this, one way or the other, but I do think it would 
be nice to make some kind of change here.

What do we think about renaming `isRValue()` to `isPRValue()` and renaming 
`VK_RValue` to `VK_PRValue`, adding a "real" `isRValue()`, and then performing 
this cleanup? I think the current state of treating "rvalue" as sometimes 
meaning rvalue and sometimes meaning "prvalue" is unhelpful and confusing, and 
this change makes it worse because `isRValue` *could* be checking for an rvalue 
whereas a comparison against `VK_RValue` more clearly is only looking for one 
specific value category rather than two.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D100733#2759592 , @aaronpuchert 
wrote:

> Not sure how to feel about this, the value categories are already hard to 
> grasp for most C++ programmers. To solve the implicit move concerns with a 
> new value category seems like cracking a nut with a sledgehammer. But that's 
> not directly related to this change, we can discuss this when the change is 
> there.

Yeah this is unrelated to this patch, but hear me out here, I agree with all 
you said about the complexity of value categories, but:

- I still think a new value category is simpler than the two phase overload 
resolution it replaces :D
- This new value category would (so far as this is only an experiment I am 
running), only apply to standards older than c++23, where from then on it's 
just the current categories and the rules as in P2266 
.
- Even in the old standards it replaces, it does not introduce any drastic 
changes. This new category is the same as XValues as far as overload resolution 
is concerned (with the small addition that it binds less preferentially to 
non-const l-value references), and as far as type deduction goes, it behaves 
exactly as LValues.

In D100733#2759898 , @aaron.ballman 
wrote:

> I'm not certain this is a big win in all cases, especially because the 
> standard has one set of terminology and we use another. e.g., the standard 
> makes a distinction between rvalue and prvalue that we don't make, same for 
> lvalue and glvalue. When I see the equality expression, I know that only one 
> category is being tested, but when I see the function call expression, I have 
> to think more about "does `isRValue()` return true for a prvalue and an 
> xvalue, or just a prvalue?"

I understand. But consider that it's not complicated right now:

  bool isLValue() const { return getValueKind() == VK_LValue; }
  bool isRValue() const { return getValueKind() == VK_RValue; }
  bool isXValue() const { return getValueKind() == VK_XValue; }
  bool isGLValue() const { return getValueKind() != VK_RValue; }

They are all testing exactly one enum, except the GLValue one. I understand 
however that I worked in very small subset of clang, this is as easy to 
remember as it gets for me, but this is not the same for others with different 
workloads. I am sure you have way more stuff to remember :)

I get @aaronpuchert 's point about how this only makes it consistent for now. I 
think we should so something about that and still make it more consistent.

How about another approach: we remove all the helpers that test exactly one 
enum (X, L and R, leaving only GL).
What do you think?

Another approach would be to document, or make more official, that the helpers 
that test a category with just one letter are just testing one enum value, and 
the ones with two or more letters are testing something more complex.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

In D100733#2759592 , @aaronpuchert 
wrote:

> @aaron.ballman, what do you think about this? We can't really prevent anyone 
> from writing `.getValueKind() == VK_*Value` instead of `.is*Value()`, so this 
> change will make things consistent only for now.

I'm not certain this is a big win in all cases, especially because the standard 
has one set of terminology and we use another. e.g., the standard makes a 
distinction between rvalue and prvalue that we don't make, same for lvalue and 
glvalue. When I see the equality expression, I know that only one category is 
being tested, but when I see the function call expression, I have to think more 
about "does `isRValue()` return true for a prvalue and an xvalue, or just a 
prvalue?"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a subscriber: aaron.ballman.
aaronpuchert added a comment.

@aaron.ballman, what do you think about this? We can't really prevent anyone 
from writing `.getValueKind() == VK_*Value` instead of `.is*Value()`, so this 
change will make things consistent only for now.

In D100733#2697540 , @mizvekov wrote:

> In D100733#2697537 , @aaronpuchert 
> wrote:
>
>> The change seems to be correct, but I'm wondering if `x.getValueKind() == 
>> VK_*Value` doesn't have one advantage over `x.is*Value()`: it's obvious that 
>> this is exclusive with the other values. Especially with `isRValue()` it 
>> might not be so obvious, because Clang doesn't follow the C++11 terminology 
>> with this.
>>
>> But it's admittedly shorter, so I'd be willing to approve this.
>
> This came up in a patch where I am experimenting with a new value category.

Not sure how to feel about this, the value categories are already hard to grasp 
for most C++ programmers. To solve the implicit move concerns with a new value 
category seems like cracking a nut with a sledgehammer. But that's not directly 
related to this change, we can discuss this when the change is there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D100733#2697537 , @aaronpuchert 
wrote:

> The change seems to be correct, but I'm wondering if `x.getValueKind() == 
> VK_*Value` doesn't have one advantage over `x.is*Value()`: it's obvious that 
> this is exclusive with the other values. Especially with `isRValue()` it 
> might not be so obvious, because Clang doesn't follow the C++11 terminology 
> with this.
>
> But it's admittedly shorter, so I'd be willing to approve this.

This came up in a patch where I am experimenting with a new value category.
The helpers 'help' a lot more when you are changing some of these tests from 
testing just one category, to testing a combination of categories (like 
GLValue).

But this is just the easy pickings of the patch, which is still WIP, and I may 
need in the future for some more drastic change to deal with the code that just 
stores the kind in a variable and tests that later.

It would be useful if you could do:

  VK = Expr->getValueKind();
  if (VK.isGLValue()) {

I may need to do something like that later.




Comment at: clang/lib/Sema/SemaExpr.cpp:5522
 }
 VK = LHSExp->getValueKind();
 if (VK != VK_RValue)

aaronpuchert wrote:
> There might be a certain benefit to using `LHSExp->getValueKind()` above when 
> we use it here again: that makes it more obvious what we're trying to achieve 
> in that `if`. (Namely changing the value category.)
Or just making the same kind of change here again:
```
if (!LHSExp->isRValue())
  OK = OK_VectorComponent;
VK = LHSExp->getValueKind();
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

The change seems to be correct, but I'm wondering if `x.getValueKind() == 
VK_*Value` doesn't have one advantage over `x.is*Value()`: it's obvious that 
this is exclusive with the other values. Especially with `isRValue()` it might 
not be so obvious, because Clang doesn't follow the C++11 terminology with this.

But it's admittedly shorter, so I'd be willing to approve this.




Comment at: clang/lib/Sema/SemaExpr.cpp:5522
 }
 VK = LHSExp->getValueKind();
 if (VK != VK_RValue)

There might be a certain benefit to using `LHSExp->getValueKind()` above when 
we use it here again: that makes it more obvious what we're trying to achieve 
in that `if`. (Namely changing the value category.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov created this revision.
Herald added a subscriber: lxfind.
mizvekov requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Signed-off-by: Matheus Izvekov 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100733

Files:
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprClassification.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCoroutine.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaInit.cpp

Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -5830,7 +5830,7 @@
  Entity.getType()) &&
 canPerformArrayCopy(Entity)) {
   // If source is a prvalue, use it directly.
-  if (Initializer->getValueKind() == VK_RValue) {
+  if (Initializer->isRValue()) {
 AddArrayInitStep(DestType, /*IsGNUExtension*/false);
 return;
   }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5513,7 +5513,7 @@
 BaseExpr = LHSExp;// vectors: V[123]
 IndexExpr = RHSExp;
 // We apply C++ DR1213 to vector subscripting too.
-if (getLangOpts().CPlusPlus11 && LHSExp->getValueKind() == VK_RValue) {
+if (getLangOpts().CPlusPlus11 && LHSExp->isRValue()) {
   ExprResult Materialized = TemporaryMaterializationConversion(LHSExp);
   if (Materialized.isInvalid())
 return ExprError();
@@ -10093,7 +10093,7 @@
 RHSType, DiagID))
 return RHSType;
 } else {
-  if (LHS.get()->getValueKind() == VK_LValue ||
+  if (LHS.get()->isLValue() ||
   !tryGCCVectorConvertAndSplat(*this, &LHS, &RHS))
 return RHSType;
 }
@@ -14816,7 +14816,7 @@
 // complex l-values to ordinary l-values and all other values to r-values.
 if (Input.isInvalid()) return ExprError();
 if (Opc == UO_Real || Input.get()->getType()->isAnyComplexType()) {
-  if (Input.get()->getValueKind() != VK_RValue &&
+  if (!Input.get()->isRValue() &&
   Input.get()->getObjectKind() == OK_Ordinary)
 VK = Input.get()->getValueKind();
 } else if (!getLangOpts().CPlusPlus) {
@@ -18985,7 +18985,7 @@
   Expr *SubExpr = SubResult.get();
   E->setSubExpr(SubExpr);
   E->setType(S.Context.getPointerType(SubExpr->getType()));
-  assert(E->getValueKind() == VK_RValue);
+  assert(E->isRValue());
   assert(E->getObjectKind() == OK_Ordinary);
   return E;
 }
@@ -18995,7 +18995,7 @@
 
   E->setType(VD->getType());
 
-  assert(E->getValueKind() == VK_RValue);
+  assert(E->isRValue());
   if (S.getLangOpts().CPlusPlus &&
   !(isa(VD) &&
 cast(VD)->isInstance()))
@@ -19086,7 +19086,7 @@
 return ExprError();
   }
 
-  assert(E->getValueKind() == VK_RValue);
+  assert(E->isRValue());
   assert(E->getObjectKind() == OK_Ordinary);
   E->setType(DestType);
 
@@ -19240,7 +19240,7 @@
 ExprResult RebuildUnknownAnyExpr::VisitImplicitCastExpr(ImplicitCastExpr *E) {
   // The only case we should ever see here is a function-to-pointer decay.
   if (E->getCastKind() == CK_FunctionToPointerDecay) {
-assert(E->getValueKind() == VK_RValue);
+assert(E->isRValue());
 assert(E->getObjectKind() == OK_Ordinary);
 
 E->setType(DestType);
@@ -19254,7 +19254,7 @@
 E->setSubExpr(Result.get());
 return E;
   } else if (E->getCastKind() == CK_LValueToRValue) {
-assert(E->getValueKind() == VK_RValue);
+assert(E->isRValue());
 assert(E->getObjectKind() == OK_Ordinary);
 
 assert(isa(E->getType()));
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -897,7 +897,7 @@
 
   // If the expression is a temporary, materialize it as an lvalue so that we
   // can use it multiple times.
-  if (E->getValueKind() == VK_RValue)
+  if (E->isRValue())
 E = CreateMaterializeTemporaryExpr(E->getType(), E, true);
 
   // The location of the `co_await` token cannot be used when constructing
@@ -957,7 +957,7 @@
 
   // If the expression is a temporary, materialize it as an lvalue so that we
   // can use it multiple times.
-  if (E->getValueKind() == VK_RValue)
+  if (E->isRValue())
 E = CreateMaterializeTemporaryExpr(E->getType(), E, true);
 
   // Build the await_ready, await_suspend, await_resume calls.
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -614,7 +614,7 @@
   // C++1z [conv.array]: The temporary materialization conversion is applied.
   // We also use this to