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

[mizvekov]

Summary

Signed-off-by: Matheus Izvekov <[e-mail]>

[aaronpuchert]

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.

clang/lib/Sema/SemaExpr.cpp

5555

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.)

[mizvekov]

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.

clang/lib/Sema/SemaExpr.cpp

5555

Or just making the same kind of change here again:

if (!LHSExp->isRValue())
  OK = OK_VectorComponent;
VK = LHSExp->getValueKind();

[aaronpuchert]

@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.

[aaron.ballman]

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?"

[mizvekov]

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 do 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.

[rsmith]

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.

[mizvekov]

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?

[aaronpuchert]

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.)

[mizvekov]

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.

[mizvekov]

Rebase.

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

[mizvekov]

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

clang/include/clang/AST/ExprCXX.h

public:
   unsigned getManglingNumber() const {
     return State.is<Stmt *>() ? 0
                               : State.get<LifetimeExtendedTemporaryDecl *>()
                                     ->getManglingNumber();
   }
 
   /// Determine whether this materialized temporary is bound to an
   /// lvalue reference; otherwise, it's bound to an rvalue reference.
-  bool isBoundToLvalueReference() const {
-    return getValueKind() == VK_LValue;
-  }
+  bool isBoundToLvalueReference() const { return isLValue(); }
 
   /// Determine whether this temporary object is usable in constant
   /// expressions, as specified in C++20 [expr.const]p4.
   bool isUsableInConstantExpressions(const ASTContext &Context) const;
 
   SourceLocation getBeginLoc() const LLVM_READONLY {
     return getSubExpr()->getBeginLoc();
   }

clang/lib/AST/Expr.cpp

 //===----------------------------------------------------------------------===//
 // Generic Expression Routines
 //===----------------------------------------------------------------------===//
 
 bool Expr::isReadIfDiscardedInCPlusPlus11() const {
   // In C++11, discarded-value expressions of a certain form are special,
   // according to [expr]p10:
   //   The lvalue-to-rvalue conversion (4.1) is applied only if the
-  //   expression is an lvalue of volatile-qualified type and it has
+  //   expression is a glvalue of volatile-qualified type and it has
   //   one of the following forms:
   if (!isGLValue() || !getType().isVolatileQualified())
     return false;
 
   const Expr *E = IgnoreParens();
 
   //   - id-expression (5.1.1),
   if (isa<DeclRefExpr>(E))
Expr::isNullPointerConstant(ASTContext &Ctx,
   return NPCK_ZeroExpression;
 }
 
 /// If this expression is an l-value for an Objective C
 /// property, find the underlying property reference expression.
 const ObjCPropertyRefExpr *Expr::getObjCProperty() const {
   const Expr *E = this;
   while (true) {
-    assert((E->getValueKind() == VK_LValue &&
-            E->getObjectKind() == OK_ObjCProperty) &&
+    assert((E->isLValue() && E->getObjectKind() == OK_ObjCProperty) &&
            "expression is not a property reference");
     E = E->IgnoreParenCasts();
     if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
       if (BO->getOpcode() == BO_Comma) {
         E = BO->getRHS();
         continue;
       }
     }
bool Expr::isObjCSelfExpr() const {
   return M->getSelfDecl() == Param;
 }
 
 FieldDecl *Expr::getSourceBitField() {
   Expr *E = this->IgnoreParens();
 
   while (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
     if (ICE->getCastKind() == CK_LValueToRValue ||
-        (ICE->getValueKind() != VK_PRValue && ICE->getCastKind() == CK_NoOp))
+        (ICE->isGLValue() && ICE->getCastKind() == CK_NoOp))
       E = ICE->getSubExpr()->IgnoreParens();
     else
       break;
   }
 
   if (MemberExpr *MemRef = dyn_cast<MemberExpr>(E))
     if (FieldDecl *Field = dyn_cast<FieldDecl>(MemRef->getMemberDecl()))
       if (Field->isBitField())
FieldDecl *Expr::getSourceBitField() {
   return nullptr;
 }
 
 bool Expr::refersToVectorElement() const {
   // FIXME: Why do we not just look at the ObjectKind here?
   const Expr *E = this->IgnoreParens();
 
   while (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
-    if (ICE->getValueKind() != VK_PRValue && ICE->getCastKind() == CK_NoOp)
+    if (ICE->isGLValue() && ICE->getCastKind() == CK_NoOp)
       E = ICE->getSubExpr()->IgnoreParens();
     else
       break;
   }
 
   if (const ArraySubscriptExpr *ASE = dyn_cast<ArraySubscriptExpr>(E))
     return ASE->getBase()->getType()->isVectorType();
 

clang/lib/AST/ExprClassification.cpp

if (!Ctx.getLangOpts().CPlusPlus) {
     // Void "lvalues" are classified as addressable void values, which are void
     // expressions whose address can be taken.
     else if (TR->isVoidType() && !TR.hasQualifiers())
       kind = (kind == Cl::CL_LValue ? Cl::CL_AddressableVoid : Cl::CL_Void);
   }
 
   // Enable this assertion for testing.
   switch (kind) {
-  case Cl::CL_LValue: assert(getValueKind() == VK_LValue); break;
-  case Cl::CL_XValue: assert(getValueKind() == VK_XValue); break;
+  case Cl::CL_LValue:
+    assert(isLValue());
+    break;
+  case Cl::CL_XValue:
+    assert(isXValue());
+    break;
   case Cl::CL_Function:
   case Cl::CL_Void:
   case Cl::CL_AddressableVoid:
   case Cl::CL_DuplicateVectorComponents:
   case Cl::CL_MemberFunction:
   case Cl::CL_SubObjCPropertySetting:
   case Cl::CL_ClassTemporary:
   case Cl::CL_ArrayTemporary:
   case Cl::CL_ObjCMessageRValue:
   case Cl::CL_PRValue:
-    assert(getValueKind() == VK_PRValue);
+    assert(isPRValue());
     break;
   }
 
   Cl::ModifiableType modifiable = Cl::CM_Untested;
   if (Loc)
     modifiable = IsModifiable(Ctx, this, kind, *Loc);
   return Classification(kind, modifiable);
 }

clang/lib/CodeGen/CGDecl.cpp

case CK_LValueToRValue: {
       // Handle a formal type change to avoid asserting.
       auto srcAddr = srcLV.getAddress(CGF);
       if (needsCast) {
         srcAddr = CGF.Builder.CreateElementBitCast(
             srcAddr, destLV.getAddress(CGF).getElementType());
       }
 
       // If it was an l-value, use objc_copyWeak.
-      if (srcExpr->getValueKind() == VK_LValue) {
+      if (srcExpr->isLValue()) {
         CGF.EmitARCCopyWeak(destLV.getAddress(CGF), srcAddr);
       } else {
-        assert(srcExpr->getValueKind() == VK_XValue);
+        assert(srcExpr->isXValue());
         CGF.EmitARCMoveWeak(destLV.getAddress(CGF), srcAddr);
       }
       return true;
     }
 
     // Stop at anything else.
     default:
       return false;

clang/lib/CodeGen/CGExprScalar.cpp

bool CodeGenFunction::ShouldNullCheckClassCastValue(const CastExpr *CE) {
 
   if (isa<CXXThisExpr>(E->IgnoreParens())) {
     // We always assume that 'this' is never null.
     return false;
   }
 
   if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(CE)) {
     // And that glvalue casts are never null.
-    if (ICE->getValueKind() != VK_PRValue)
+    if (ICE->isGLValue())
       return false;
   }
 
   return true;
 }
 
 // VisitCastExpr - Emit code for an explicit or implicit cast.  Implicit casts
 // have to handle a more broad range of conversions than explicit casts, as they

clang/lib/Sema/Sema.cpp

#endif
   QualType TypeTy = Context.getCanonicalType(Ty);
 
   if (ExprTy == TypeTy)
     return E;
 
   if (Kind == CK_ArrayToPointerDecay) {
     // C++1z [conv.array]: The temporary materialization conversion is applied.
     // We also use this to fuel C++ DR1213, which applies to C++11 onwards.
-    if (getLangOpts().CPlusPlus && E->getValueKind() == VK_PRValue) {
+    if (getLangOpts().CPlusPlus && E->isPRValue()) {
       // The temporary is an lvalue in C++98 and an xvalue otherwise.
       ExprResult Materialized = CreateMaterializeTemporaryExpr(
           E->getType(), E, !getLangOpts().CPlusPlus11);
       if (Materialized.isInvalid())
         return ExprError();
       E = Materialized.get();
     }
     // C17 6.7.1p6 footnote 124: The implementation can treat any register

clang/lib/Sema/SemaCoroutine.cpp

ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *E,
   if (E->getType()->isDependentType()) {
     Expr *Res = new (Context)
         CoawaitExpr(Loc, Context.DependentTy, E, IsImplicit);
     return Res;
   }
 
   // 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
   // the member call expressions since it's before the location of `Expr`, which
   // is used as the start of the member call expression.
   SourceLocation CallLoc = E->getExprLoc();
 
   // Build the await_ready, await_suspend, await_resume calls.
ExprResult Sema::BuildCoyieldExpr(SourceLocation Loc, Expr *E) {
 
   if (E->getType()->isDependentType()) {
     Expr *Res = new (Context) CoyieldExpr(Loc, Context.DependentTy, E);
     return Res;
   }
 
   // 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);
 
   // Build the await_ready, await_suspend, await_resume calls.
   ReadySuspendResumeResult RSS = buildCoawaitCalls(
       *this, Coroutine->CoroutinePromise, Loc, E);
   if (RSS.IsInvalid)
     return ExprError();
 

clang/lib/Sema/SemaExpr.cpp

if (!LangOpts.isSubscriptPointerArithmetic()) {
       Diag(LLoc, diag::err_subscript_nonfragile_interface)
         << ResultType << BaseExpr->getSourceRange();
       return ExprError();
     }
   } else if (const VectorType *VTy = LHSTy->getAs<VectorType>()) {
     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();
       LHSExp = Materialized.get();
     }
     VK = LHSExp->getValueKind();
     if (VK != VK_PRValue)
       OK = OK_VectorComponent;
 
     ResultType = VTy->getElementType();
     QualType BaseType = BaseExpr->getType();
     Qualifiers BaseQuals = BaseType.getQualifiers();
     Qualifiers MemberQuals = ResultType.getQualifiers();
     Qualifiers Combined = BaseQuals + MemberQuals;
QualType Sema::CheckVectorOperands(ExprResult &LHS, ExprResult &RHS,
   }
   if (!LHSVecType) {
     if (isa<ExtVectorType>(RHSVecType)) {
       if (!tryVectorConvertAndSplat(*this, (IsCompAssign ? nullptr : &LHS),
                                     LHSType, RHSVecType->getElementType(),
                                     RHSType, DiagID))
         return RHSType;
     } else {
-      if (LHS.get()->getValueKind() == VK_LValue ||
+      if (LHS.get()->isLValue() ||
           !tryGCCVectorConvertAndSplat(*this, &LHS, &RHS))
         return RHSType;
     }
   }
 
   // FIXME: The code below also handles conversion between vectors and
   // non-scalars, we should break this down into fine grained specific checks
   // and emit proper diagnostics.
case UO_LNot: // logical negation
     break;
   case UO_Real:
   case UO_Imag:
     resultType = CheckRealImagOperand(*this, Input, OpLoc, Opc == UO_Real);
     // _Real maps ordinary l-values into ordinary l-values. _Imag maps ordinary
     // 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) {
       // In C, a volatile scalar is read by __imag. In C++, it is not.
       Input = DefaultLvalueConversion(Input.get());
     }
     break;
   case UO_Extension:
struct RebuildUnknownAnyFunction
 
     ExprResult VisitUnaryAddrOf(UnaryOperator *E) {
       ExprResult SubResult = Visit(E->getSubExpr());
       if (SubResult.isInvalid()) return ExprError();
 
       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;
     }
 
     ExprResult resolveDecl(Expr *E, ValueDecl *VD) {
       if (!isa<FunctionDecl>(VD)) return VisitExpr(E);
 
       E->setType(VD->getType());
 
-      assert(E->getValueKind() == VK_PRValue);
+      assert(E->isPRValue());
       if (S.getLangOpts().CPlusPlus &&
           !(isa<CXXMethodDecl>(VD) &&
             cast<CXXMethodDecl>(VD)->isInstance()))
         E->setValueKind(VK_LValue);
 
       return E;
     }
 
ExprResult VisitUnaryAddrOf(UnaryOperator *E) {
       }
 
       if (isa<CallExpr>(E->getSubExpr())) {
         S.Diag(E->getOperatorLoc(), diag::err_unknown_any_addrof_call)
           << E->getSourceRange();
         return ExprError();
       }
 
-      assert(E->getValueKind() == VK_PRValue);
+      assert(E->isPRValue());
       assert(E->getObjectKind() == OK_Ordinary);
       E->setType(DestType);
 
       // Build the sub-expression as if it were an object of the pointee type.
       DestType = Ptr->getPointeeType();
       ExprResult SubResult = Visit(E->getSubExpr());
       if (SubResult.isInvalid()) return ExprError();
       E->setSubExpr(SubResult.get());
ExprResult RebuildUnknownAnyExpr::VisitObjCMessageExpr(ObjCMessageExpr *E) {
   E->setValueKind(Expr::getValueKindForType(DestType));
 
   return S.MaybeBindToTemporary(E);
 }
 
 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);
 
     // Rebuild the sub-expression as the pointee (function) type.
     DestType = DestType->castAs<PointerType>()->getPointeeType();
 
     ExprResult Result = Visit(E->getSubExpr());
     if (!Result.isUsable()) return ExprError();
 
     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<BlockPointerType>(E->getType()));
 
     E->setType(DestType);
 
     // The sub-expression has to be a lvalue reference, so rebuild it as such.
     DestType = S.Context.getLValueReferenceType(DestType);

clang/lib/Sema/SemaExprCXX.cpp

 
 ExprResult Sema::MaybeBindToTemporary(Expr *E) {
   if (!E)
     return ExprError();
 
   assert(!isa<CXXBindTemporaryExpr>(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,
   // in which case we have to insert a consuming cast.
   if (getLangOpts().ObjCAutoRefCount &&
       E->getType()->isObjCRetainableType()) {
 
     bool ReturnsRetained;

clang/lib/Sema/SemaInit.cpp

if (const ArrayType *DestAT = Context.getAsArrayType(DestType)) {
 
     // Some kinds of initialization permit an array to be initialized from
     // another array of the same type, and perform elementwise initialization.
     if (Initializer && isa<ConstantArrayType>(DestAT) &&
         S.Context.hasSameUnqualifiedType(Initializer->getType(),
                                          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;
       }
 
       // Emit element-at-a-time copy loop.
       InitializedEntity Element =
           InitializedEntity::InitializeElement(S.Context, 0, Entity);
       QualType InitEltT =