[Sema] Implement C++14's DR1579: Prefer moving id-expression out of functions

[erik.pilkington]

Summary

DR1579 says that when returning a id-expression from a function, we should attempt to return said expression by move first, then fallback to copy.

This patch does this by generalizing PerformMoveOrCopyElision, which previously did this when returning a id-expression that was a parameter.

This patch allows the following code to compile, for example:

struct Base {};
struct Derived : public Base {};
std::unique_ptr<Base> f() {
  std::unique_ptr<Derived> d;
  return d;
}

The DR in question: http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579
Pointed out by PR27785.

Thanks!

[rsmith]

Thank you for working on this! Please also add a test to test/CXX/drs/dr15xx.cpp with a "// dr1579: 3.9" comment (we have a script that turns those comments into www/cxx_dr_status.html).

include/clang/Sema/Sema.h

3473 ↗

(On Diff #61589)

Constructable -> Constructible.

lib/Sema/SemaStmt.cpp

2756–2759 ↗

(On Diff #61589)

Combine these conditions into a single if, and put the (cheapest) bool check first.

2773–2783 ↗

(On Diff #61589)

None of this should be checked for the AllowParamOrMoveConstructible case.

2820 ↗

(On Diff #61589)

Why are you using direct-initialization here? The return value should always be copy-initialized.

[erik.pilkington]

This new patch addresses all of Richard's comments.

[rsmith]

LGTM, thank you!

[mehdi_amini]

Is it also PR23849? Is it dup with PR27785?

[erik.pilkington]

Yes, also PR28096. I marked them as being duplicates on the bugzilla.

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

public:
 
   /// \brief Determine whether the initialization sequence is invalid.
   bool Failed() const { return SequenceKind == FailedSequence; }
 
   typedef SmallVectorImpl<Step>::const_iterator step_iterator;
   step_iterator step_begin() const { return Steps.begin(); }
   step_iterator step_end()   const { return Steps.end(); }
 
+  typedef llvm::iterator_range<step_iterator> step_range;
+  step_range steps() const { return {step_begin(), step_end()}; }
+
   /// \brief Determine whether this initialization is a direct reference 
   /// binding (C++ [dcl.init.ref]).
   bool isDirectReferenceBinding() const;
   
   /// \brief Determine whether this initialization failed due to an ambiguity.
   bool isAmbiguous() const;
   
   /// \brief Determine whether this initialization is direct call to a 

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

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);
   VarDecl *getCopyElisionCandidate(QualType ReturnType, Expr *E,
-                                   bool AllowFunctionParameters);
+                                   bool AllowParamOrMoveConstructible);
   bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-                              bool AllowFunctionParameters);
+                              bool AllowParamOrMoveConstructible);
 
   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/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 AllowFunctionParameter Whether we allow function parameters to
-/// be considered NRVO candidates. C++ prohibits this 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).
+/// \param AllowParamOrMoveConstructible 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 AllowFunctionParameter) {
+VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, Expr *E,
+                                       bool AllowParamOrMoveConstructible) {
   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, AllowFunctionParameter))
+  if (isCopyElisionCandidate(ReturnType, VD, AllowParamOrMoveConstructible))
     return VD;
   return nullptr;
 }
 
 bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-                                  bool AllowFunctionParameter) {
+                                  bool AllowParamOrMoveConstructible) {
   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 ...
-    if (!VDType->isDependentType() &&
+    // When considering moving this expression out, allow dissimilar types.
+    if (!AllowParamOrMoveConstructible && !VDType->isDependentType() &&
         !Context.hasSameUnqualifiedType(ReturnType, VDType))
       return false;
   }
 
   // ...object (other than a function or catch-clause parameter)...
   if (VD->getKind() != Decl::Var &&
-      !(AllowFunctionParameter && VD->getKind() == Decl::ParmVar))
+      !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
     return false;
   if (VD->isExceptionVariable()) return false;
 
   // ...automatic...
   if (!VD->hasLocalStorage()) return false;
 
+  if (AllowParamOrMoveConstructible)
+    return true;
+
   // ...non-volatile...
   if (VD->getType().isVolatileQualified()) return false;
 
   // __block variables can't be allocated in a way that permits NRVO.
   if (VD->hasAttr<BlocksAttr>()) 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.
 ///
-/// This routine implements C++0x [class.copy]p33, which attempts to treat
+/// 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++0x [class.copy]p33:
-  //   When the criteria for elision of a copy operation are met or would
-  //   be met save for the fact that the source object is a function
-  //   parameter, and the object to be copied is designated by an lvalue,
-  //   overload resolution to select the constructor for the copy is first
-  //   performed as if the object were designated by an rvalue.
+  // 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 || getCopyElisionCandidate(ResultType, Value, true))) {
-    ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack,
-                              Value->getType(), CK_NoOp, Value, VK_XValue);
+
+  if (AllowNRVO && !NRVOCandidate)
+    NRVOCandidate = getCopyElisionCandidate(ResultType, Value, true);
+
+  if (AllowNRVO && NRVOCandidate) {
+    ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
+                              CK_NoOp, Value, VK_XValue);
 
     Expr *InitExpr = &AsRvalue;
-    InitializationKind Kind
-      = InitializationKind::CreateCopy(Value->getLocStart(),
-                                       Value->getLocStart());
-    InitializationSequence Seq(*this, Entity, Kind, InitExpr);
 
-    //   [...] If overload resolution fails, 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.
+    InitializationKind Kind = InitializationKind::CreateCopy(
+        Value->getLocStart(), Value->getLocStart());
+
+    InitializationSequence Seq(*this, Entity, Kind, InitExpr);
     if (Seq) {
-      for (InitializationSequence::step_iterator Step = Seq.step_begin(),
-           StepEnd = Seq.step_end();
-           Step != StepEnd; ++Step) {
-        if (Step->Kind != InitializationSequence::SK_ConstructorInitialization)
+      for (const InitializationSequence::Step &Step : Seq.steps()) {
+        if (!(Step.Kind ==
+                  InitializationSequence::SK_ConstructorInitialization ||
+              (Step.Kind == InitializationSequence::SK_UserConversion &&
+               isa<CXXConstructorDecl>(Step.Function.Function))))
           continue;
 
-        CXXConstructorDecl *Constructor
-        = cast<CXXConstructorDecl>(Step->Function.Function);
+        CXXConstructorDecl *Constructor =
+            cast<CXXConstructorDecl>(Step.Function.Function);
 
         const RValueReferenceType *RRefType
           = Constructor->getParamDecl(0)->getType()
                                                  ->getAs<RValueReferenceType>();
 
-        // If we don't meet the criteria, break out now.
+        // [...] 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(),
-                            Context.getTypeDeclType(Constructor->getParent())))
+                                            NRVOCandidate->getType()))
           break;
 
         // 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(Context, Value->getType(), CK_NoOp,
+                                         Value, nullptr, VK_XValue);
 
         // Complete type-checking the initialization of the return type
         // using the constructor we found.
         Res = Seq.Perform(*this, Entity, Kind, Value);
       }
     }
   }
 

cfe/trunk/test/CXX/drs/dr15xx.cpp

namespace std {
   template<typename T> struct basic_string {
     basic_string(const T* x) {}
     ~basic_string() {};
   };
   typedef basic_string<char> string;
 
 } // std
 
+namespace dr1579 { // dr1579: 3.9
+template<class T>
+struct GenericMoveOnly {
+  GenericMoveOnly();
+  template<class U> GenericMoveOnly(const GenericMoveOnly<U> &) = delete; // expected-note 5 {{marked deleted here}}
+  GenericMoveOnly(const int &) = delete; // expected-note 2 {{marked deleted here}}
+  template<class U> GenericMoveOnly(GenericMoveOnly<U> &&);
+  GenericMoveOnly(int &&);
+};
+
+GenericMoveOnly<float> DR1579_Eligible(GenericMoveOnly<char> CharMO) {
+  int i;
+  GenericMoveOnly<char> GMO;
+
+  if (0)
+    return i;
+  else if (0)
+    return GMO;
+  else if (0)
+    return ((GMO));
+  else
+    return CharMO;
+}
+
+GenericMoveOnly<char> GlobalMO;
+
+GenericMoveOnly<float> DR1579_Ineligible(int &AnInt,
+                                          GenericMoveOnly<char> &CharMO) {
+  static GenericMoveOnly<char> StaticMove;
+  extern GenericMoveOnly<char> ExternMove;
+
+  if (0)
+    return AnInt; // expected-error{{invokes a deleted function}}
+  else if (0)
+    return GlobalMO; // expected-error{{invokes a deleted function}}
+  else if (0)
+    return StaticMove; // expected-error{{invokes a deleted function}}
+  else if (0)
+    return ExternMove; // expected-error{{invokes a deleted function}}
+  else if (0)
+    return AnInt; // expected-error{{invokes a deleted function}}
+  else
+    return CharMO; // expected-error{{invokes a deleted function}}
+}
+
+auto DR1579_lambda_valid = [](GenericMoveOnly<float> mo) ->
+  GenericMoveOnly<char> {
+  return mo;
+};
+
+auto DR1579_lambda_invalid = []() -> GenericMoveOnly<char> {
+  static GenericMoveOnly<float> mo;
+  return mo; // expected-error{{invokes a deleted function}}
+};
+} // end namespace dr1579
+
 namespace dr1589 {   // dr1589: 3.7 c++11
   // Ambiguous ranking of list-initialization sequences
 
   void f0(long, int=0);                 // Would makes selection of #0 ambiguous
   void f0(long);                        // #0
   void f0(std::initializer_list<int>);  // #00
   void g0() { f0({1L}); }               // chooses #00
 

cfe/trunk/test/SemaCXX/rval-references.cpp

 int&& should_not_warn(int&& i) { // But GCC 4.4 does
   return static_cast<int&&>(i);
 }
 
 
 // Test the return dance. This also tests IsReturnCopyElidable.
 struct MoveOnly {
   MoveOnly();
-  MoveOnly(const MoveOnly&) = delete;  // expected-note {{candidate constructor}} \
-  // expected-note 3{{explicitly marked deleted here}}
-  MoveOnly(MoveOnly&&); // expected-note {{candidate constructor}}
-  MoveOnly(int&&);  // expected-note {{candidate constructor}}
+  MoveOnly(const MoveOnly&) = delete;  // expected-note 3{{explicitly marked deleted here}}
 };
 
 MoveOnly gmo;
 MoveOnly returningNonEligible() {
-  int i;
   static MoveOnly mo;
   MoveOnly &r = mo;
   if (0) // Copy from global can't be elided
     return gmo; // expected-error {{call to deleted constructor}}
   else if (0) // Copy from local static can't be elided
     return mo; // expected-error {{call to deleted constructor}}
-  else if (0) // Copy from reference can't be elided
+  else // Copy from reference can't be elided
     return r; // expected-error {{call to deleted constructor}}
-  else // Construction from different type can't be elided
-    return i; // expected-error {{no viable conversion from returned value of type 'int' to function return type 'MoveOnly'}}
 }