Preliminary refactoring in service of -Wreturn-std-move. NFC.

[• Quuxplusone]

Summary

This patch is extracted from D43322, which adds a new diagnostic -Wreturn-std-move. This patch here is just the non-functional parts of that patch. It pulls TryMoveInitialization out into a separate function so that we can (in the next patch) try it twice — once with current C++ rules, and once with the rules as they would be if return x considered rvalue-ref-qualified conversion operators. This patch here does *not* add those new rules; it merely rearranges the existing code to make the next patch less bulky.

[• Quuxplusone]

Add a block comment for function TryMoveInitialization.

[• Quuxplusone]

@rtrieu please take a look?

Also, I would need someone to commit this for me, as I don't have commit privs.

[• Quuxplusone]

@rtrieu ping?

[rtrieu]

lib/Sema/SemaStmt.cpp

2953 ↗

(On Diff #136380)

Opening brace should follow the closing paren on previous line.

2963 ↗

(On Diff #136380)

Use early exit here:

if (!Seq)
  return;

// rest of function

2965–2966 ↗

(On Diff #136380)

Since you're simplifying the condition here, bring the not operator inside the parentheses.

if (Step.Kind != ...  && Step.Kind != ...)

2970 ↗

(On Diff #136380)

Use early exit here:

if (!isa<CXXConstructorDecl>(FD)
  continue;

// old if-block code

2999–3000 ↗

(On Diff #136380)

At this point, the variable Value is updated. Value is scoped to this function, and used again after this code. In your change, there is a new Value variable in the static function. Only that variable is updated and not this one, making this a change in behavior.

[• Quuxplusone]

lib/Sema/SemaStmt.cpp

2970 ↗

(On Diff #136380)

I'd prefer not to do this, since D43322 is going to change this code into "if isa-red-fish... else if isa-blue-fish...". Therefore I think it makes sense to keep this intermediate stage as "if isa-red-fish...", rather than changing it into "if not-isa-red-fish continue... otherwise..."

If you really want this, I can change it; but it's just going to change back in D43322, and the goal of this patch was to make D43322 smaller.

2999–3000 ↗

(On Diff #136380)

Good catch! I've addressed this now by making the parameter Expr *&Value; but I'd be open to better approaches. Particularly because I still don't know what to do about the "unnecessary promoting Value to the heap" that will happen in D43322.

[• Quuxplusone]

Addressed @rtrieu's comments.

@rtrieu, please take another look at this and D43322? Thanks!

[rtrieu]

Everything looks good and ready for commit.

[• Quuxplusone]

@rtrieu thanks! I don't have commit privileges; could I ask you to commit this on my behalf? (Or if not, please say no, and I'll know to go looking for someone else to ask.)

Once/if D43898 hits master, I'll rebase D43322 and await a LGTM there, at which point I'll again ask for help committing it. :)

cfe/trunk/include/clang/Sema/Sema.h

public:
   void ActOnCapturedRegionStart(SourceLocation Loc, Scope *CurScope,
                                 CapturedRegionKind Kind,
                                 ArrayRef<CapturedParamNameType> Params);
   StmtResult ActOnCapturedRegionEnd(Stmt *S);
   void ActOnCapturedRegionError();
   RecordDecl *CreateCapturedStmtRecordDecl(CapturedDecl *&CD,
                                            SourceLocation Loc,
                                            unsigned NumParams);
+
+  enum CopyElisionSemanticsKind {
+    CES_Strict = 0,
+    CES_AllowParameters = 1,
+    CES_AllowDifferentTypes = 2,
+    CES_Default = (CES_AllowParameters | CES_AllowDifferentTypes),
+  };
+
   VarDecl *getCopyElisionCandidate(QualType ReturnType, Expr *E,
-                                   bool AllowParamOrMoveConstructible);
+                                   CopyElisionSemanticsKind CESK);
   bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-                              bool AllowParamOrMoveConstructible);
+                              CopyElisionSemanticsKind CESK);
 
   StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                              Scope *CurScope);
   StmtResult BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp);
   StmtResult ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp);
 
   StmtResult ActOnGCCAsmStmt(SourceLocation AsmLoc, bool IsSimple,
                              bool IsVolatile, unsigned NumOutputs,

cfe/trunk/lib/Sema/SemaExprCXX.cpp

if (Ex && !Ex->isTypeDependent()) {
     //       catch-clause
     //       parameter) whose scope does not extend beyond the end of the
     //       innermost enclosing try-block (if there is one), the copy/move
     //       operation from the operand to the exception object (15.1) can be
     //       omitted by constructing the automatic object directly into the
     //       exception object
     const VarDecl *NRVOVariable = nullptr;
     if (IsThrownVarInScope)
-      NRVOVariable = getCopyElisionCandidate(QualType(), Ex, false);
+      NRVOVariable = getCopyElisionCandidate(QualType(), Ex, CES_Strict);
 
     InitializedEntity Entity = InitializedEntity::InitializeException(
         OpLoc, ExceptionObjectTy,
         /*NRVO=*/NRVOVariable != nullptr);
     ExprResult Res = PerformMoveOrCopyInitialization(
         Entity, NRVOVariable, QualType(), Ex, IsThrownVarInScope);
     if (Res.isInvalid())
       return ExprError();

cfe/trunk/lib/Sema/SemaStmt.cpp

 /// \param ReturnType If we're determining the copy elision candidate for
 /// a return statement, this is the return type of the function. If we're
 /// determining the copy elision candidate for a throw expression, this will
 /// be a NULL type.
 ///
 /// \param E The expression being returned from the function or block, or
 /// being thrown.
 ///
-/// \param AllowParamOrMoveConstructible Whether we allow function parameters or
+/// \param CESK Whether we allow function parameters or
 /// id-expressions that could be moved out of the function to be considered NRVO
 /// candidates. C++ prohibits these for NRVO itself, but we re-use this logic to
 /// determine whether we should try to move as part of a return or throw (which
 /// does allow function parameters).
 ///
 /// \returns The NRVO candidate variable, if the return statement may use the
 /// NRVO, or NULL if there is no such candidate.
 VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, Expr *E,
-                                       bool AllowParamOrMoveConstructible) {
+                                       CopyElisionSemanticsKind CESK) {
   if (!getLangOpts().CPlusPlus)
     return nullptr;
 
   // - in a return statement in a function [where] ...
   // ... the expression is the name of a non-volatile automatic object ...
   DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E->IgnoreParens());
   if (!DR || DR->refersToEnclosingVariableOrCapture())
     return nullptr;
   VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl());
   if (!VD)
     return nullptr;
 
-  if (isCopyElisionCandidate(ReturnType, VD, AllowParamOrMoveConstructible))
+  if (isCopyElisionCandidate(ReturnType, VD, CESK))
     return VD;
   return nullptr;
 }
 
 bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-                                  bool AllowParamOrMoveConstructible) {
+                                  CopyElisionSemanticsKind CESK) {
   QualType VDType = VD->getType();
   // - in a return statement in a function with ...
   // ... a class return type ...
   if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
     if (!ReturnType->isRecordType())
       return false;
     // ... the same cv-unqualified type as the function return type ...
     // When considering moving this expression out, allow dissimilar types.
-    if (!AllowParamOrMoveConstructible && !VDType->isDependentType() &&
+    if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
         !Context.hasSameUnqualifiedType(ReturnType, VDType))
       return false;
   }
 
   // ...object (other than a function or catch-clause parameter)...
   if (VD->getKind() != Decl::Var &&
-      !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
+      !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
     return false;
   if (VD->isExceptionVariable()) return false;
 
   // ...automatic...
   if (!VD->hasLocalStorage()) return false;
 
   // Return false if VD is a __block variable. We don't want to implicitly move
   // out of a __block variable during a return because we cannot assume the
   // variable will no longer be used.
   if (VD->hasAttr<BlocksAttr>()) return false;
 
-  if (AllowParamOrMoveConstructible)
+  if (CESK & CES_AllowDifferentTypes)
     return true;
 
   // ...non-volatile...
   if (VD->getType().isVolatileQualified()) return false;
 
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
   if (!VD->getType()->isDependentType() && VD->hasAttr<AlignedAttr>() &&
       Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VD->getType()))
     return false;
 
   return true;
 }
 
-/// \brief Perform the initialization of a potentially-movable value, which
-/// is the result of return value.
+/// \brief Try to perform the initialization of a potentially-movable value,
+/// which is the operand to a return or throw statement.
 ///
 /// This routine implements C++14 [class.copy]p32, which attempts to treat
 /// returned lvalues as rvalues in certain cases (to prefer move construction),
 /// then falls back to treating them as lvalues if that failed.
-ExprResult
-Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
-                                      const VarDecl *NRVOCandidate,
-                                      QualType ResultType,
-                                      Expr *Value,
-                                      bool AllowNRVO) {
-  // C++14 [class.copy]p32:
-  // When the criteria for elision of a copy/move operation are met, but not for
-  // an exception-declaration, and the object to be copied is designated by an
-  // lvalue, or when the expression in a return statement is a (possibly
-  // parenthesized) id-expression that names an object with automatic storage
-  // duration declared in the body or parameter-declaration-clause of the
-  // innermost enclosing function or lambda-expression, overload resolution to
-  // select the constructor for the copy is first performed as if the object
-  // were designated by an rvalue.
-  ExprResult Res = ExprError();
-
-  if (AllowNRVO && !NRVOCandidate)
-    NRVOCandidate = getCopyElisionCandidate(ResultType, Value, true);
-
-  if (AllowNRVO && NRVOCandidate) {
-    ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
-                              CK_NoOp, Value, VK_XValue);
+///
+/// \param Res We will fill this in if move-initialization was possible.
+/// If move-initialization is not possible, such that we must fall back to
+/// treating the operand as an lvalue, we will leave Res in its original
+/// invalid state.
+static void TryMoveInitialization(Sema& S,
+                                  const InitializedEntity &Entity,
+                                  Expr *&Value,
+                                  ExprResult &Res) {
 
-    Expr *InitExpr = &AsRvalue;
 
-    InitializationKind Kind = InitializationKind::CreateCopy(
-        Value->getLocStart(), Value->getLocStart());
 
-    InitializationSequence Seq(*this, Entity, Kind, InitExpr);
-    if (Seq) {
-      for (const InitializationSequence::Step &Step : Seq.steps()) {
-        if (!(Step.Kind ==
-                  InitializationSequence::SK_ConstructorInitialization ||
-              (Step.Kind == InitializationSequence::SK_UserConversion &&
-               isa<CXXConstructorDecl>(Step.Function.Function))))
-          continue;
+  InitializationSequence Seq(S, Entity, Kind, InitExpr);
+
+  if (!Seq)
+    return;
+
+    if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
+        Step.Kind != InitializationSequence::SK_UserConversion)
 
-        CXXConstructorDecl *Constructor =
-            cast<CXXConstructorDecl>(Step.Function.Function);
-
-        const RValueReferenceType *RRefType
-          = Constructor->getParamDecl(0)->getType()
-                                                 ->getAs<RValueReferenceType>();
-
-        // [...] If the first overload resolution fails or was not performed, or
-        // if the type of the first parameter of the selected constructor is not
-        // an rvalue reference to the object's type (possibly cv-qualified),
-        // overload resolution is performed again, considering the object as an
-        // lvalue.
-        if (!RRefType ||
-            !Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
+    FunctionDecl *FD = Step.Function.Function;
+    if (isa<CXXConstructorDecl>(FD)) {
+      // C++14 [class.copy]p32:
+      // [...] If the first overload resolution fails or was not performed,
+      // or if the type of the first parameter of the selected constructor
+      // is not an rvalue reference to the object's type (possibly
+      // cv-qualified), overload resolution is performed again, considering
+      // the object as an lvalue.
+      const RValueReferenceType *RRefType =
+          FD->getParamDecl(0)->getType()->getAs<RValueReferenceType>();
+      if (!RRefType)
+        break;
+      if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
                                             NRVOCandidate->getType()))
-          break;
+    } else {
+      continue;
+    }
 
-        // Promote "AsRvalue" to the heap, since we now need this
-        // expression node to persist.
-        Value = ImplicitCastExpr::Create(Context, Value->getType(), CK_NoOp,
-                                         Value, nullptr, VK_XValue);
+    Value = ImplicitCastExpr::Create(S.Context, Value->getType(), CK_NoOp,
 
-        // Complete type-checking the initialization of the return type
-        // using the constructor we found.
-        Res = Seq.Perform(*this, Entity, Kind, Value);
-      }
-    }
+    Res = Seq.Perform(S, Entity, Kind, Value);
+
+/// \brief Perform the initialization of a potentially-movable value, which
+/// is the result of return value.
+///
+/// This routine implements C++14 [class.copy]p32, which attempts to treat
+/// returned lvalues as rvalues in certain cases (to prefer move construction),
+/// then falls back to treating them as lvalues if that failed.
+ExprResult
+Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity,
+                                      const VarDecl *NRVOCandidate,
+                                      QualType ResultType,
+                                      Expr *Value,
+                                      bool AllowNRVO) {
+  // C++14 [class.copy]p32:
+  // When the criteria for elision of a copy/move operation are met, but not for
+  // an exception-declaration, and the object to be copied is designated by an
+  // lvalue, or when the expression in a return statement is a (possibly
+  // parenthesized) id-expression that names an object with automatic storage
+  // duration declared in the body or parameter-declaration-clause of the
+  // innermost enclosing function or lambda-expression, overload resolution to
+  // select the constructor for the copy is first performed as if the object
+  // were designated by an rvalue.
+  ExprResult Res = ExprError();
+
+  if (AllowNRVO) {
+    if (!NRVOCandidate) {
+      NRVOCandidate = getCopyElisionCandidate(ResultType, Value, CES_Default);
+    }
+
+    if (NRVOCandidate) {
+      TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
+                            Res);
+    }
   }
 
   // Either we didn't meet the criteria for treating an lvalue as an rvalue,
   // above, or overload resolution failed. Either way, we need to try
   // (again) now with the return value expression as written.
   if (Res.isInvalid())
     Res = PerformCopyInitialization(Entity, SourceLocation(), Value);
 
if (FnRetType->isDependentType()) {
     // we have a non-void block with an expression, continue checking
 
     // C99 6.8.6.4p3(136): The return statement is not an assignment. The
     // overlap restriction of subclause 6.5.16.1 does not apply to the case of
     // function return.
 
     // In C++ the return statement is handled via a copy initialization.
     // the C version of which boils down to CheckSingleAssignmentConstraints.
-    NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false);
+    NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, CES_Strict);
     InitializedEntity Entity = InitializedEntity::InitializeResult(ReturnLoc,
                                                                    FnRetType,
                                                       NRVOCandidate != nullptr);
     ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRVOCandidate,
                                                      FnRetType, RetValExp);
     if (Res.isInvalid()) {
       // FIXME: Cleanup temporaries here, anyway?
       return StmtError();
     }
     RetValExp = Res.get();
     CheckReturnValExpr(RetValExp, FnRetType, ReturnLoc);
   } else {
-    NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false);
+    NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, CES_Strict);
   }
 
   if (RetValExp) {
     ExprResult ER = ActOnFinishFullExpr(RetValExp, ReturnLoc);
     if (ER.isInvalid())
       return StmtError();
     RetValExp = ER.get();
   }
if (FnRetType->isVoidType()) {
 
     // C99 6.8.6.4p3(136): The return statement is not an assignment. The
     // overlap restriction of subclause 6.5.16.1 does not apply to the case of
     // function return.
 
     // In C++ the return statement is handled via a copy initialization,
     // the C version of which boils down to CheckSingleAssignmentConstraints.
     if (RetValExp)
-      NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false);
+      NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, CES_Strict);
     if (!HasDependentReturnType && !RetValExp->isTypeDependent()) {
       // we have a non-void function with an expression, continue checking
       InitializedEntity Entity = InitializedEntity::InitializeResult(ReturnLoc,
                                                                      RetType,
                                                       NRVOCandidate != nullptr);
       ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRVOCandidate,
                                                        RetType, RetValExp);
       if (Res.isInvalid()) {

cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp

Decl *TemplateDeclInstantiator::VisitVarDecl(VarDecl *D,
   if (SubstQualifier(D, Var))
     return nullptr;
 
   SemaRef.BuildVariableInstantiation(Var, D, TemplateArgs, LateAttrs, Owner,
                                      StartingScope, InstantiatingVarTemplate);
 
   if (D->isNRVOVariable()) {
     QualType ReturnType = cast<FunctionDecl>(DC)->getReturnType();
-    if (SemaRef.isCopyElisionCandidate(ReturnType, Var, false))
+    if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
       Var->setNRVOVariable(true);
   }
 
   Var->setImplicit(D->isImplicit());
 
   return Var;
 }