[clang] 5f1de9c - [C++20] [P1825] Fix bugs with implicit-move from variables of reference type.

[Arthur O'Dwyer via cfe-commits]

Author: Arthur O'Dwyer
Date: 2021-03-23T14:12:06-04:00
New Revision: 5f1de9cab1ce2fbb1e45239d3e0e8d4970d2124e

URL: https://github.com/llvm/llvm-project/commit/5f1de9cab1ce2fbb1e45239d3e0e8d4970d2124e
DIFF: https://github.com/llvm/llvm-project/commit/5f1de9cab1ce2fbb1e45239d3e0e8d4970d2124e.diff

LOG: [C++20] [P1825] Fix bugs with implicit-move from variables of reference type.

Review D88220 turns out to have some pretty severe bugs, but I *think*
this patch fixes them.

Paper P1825 is supposed to enable implicit move from "non-volatile objects
and rvalue references to non-volatile object types." Instead, what was committed
seems to have enabled implicit move from "non-volatile things of all kinds,
except that if they're rvalue references then they must also refer to non-volatile
things." In other words, D88220 accidentally enabled implicit move from
lvalue object references (super yikes!) and also from non-object references
(such as references to functions).

These two cases are now fixed and regression-tested.

Differential Revision: https://reviews.llvm.org/D98971

Added: 
    

Modified: 
    clang/lib/Sema/SemaStmt.cpp
    clang/test/CXX/class/class.init/class.copy.elision/p3.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 4a275e6bd2a1..ceba83bcd814 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3092,24 +3092,30 @@ bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
   if (VD->hasAttr<BlocksAttr>())
     return false;
 
-  // ...non-volatile...
-  if (VD->getType().isVolatileQualified())
-    return false;
-
-  // C++20 [class.copy.elision]p3:
-  // ...rvalue reference to a non-volatile...
-  if (VD->getType()->isRValueReferenceType() &&
-      (!(CESK & CES_AllowRValueReferenceType) ||
-       VD->getType().getNonReferenceType().isVolatileQualified()))
+  if (VDType->isObjectType()) {
+    // C++17 [class.copy.elision]p3:
+    // ...non-volatile automatic object...
+    if (VDType.isVolatileQualified())
+      return false;
+  } else if (VDType->isRValueReferenceType()) {
+    // C++20 [class.copy.elision]p3:
+    // ...either a non-volatile object or an rvalue reference to a non-volatile object type...
+    if (!(CESK & CES_AllowRValueReferenceType))
+      return false;
+    QualType VDReferencedType = VDType.getNonReferenceType();
+    if (VDReferencedType.isVolatileQualified() || !VDReferencedType->isObjectType())
+      return false;
+  } else {
     return false;
+  }
 
   if (CESK & CES_AllowDifferentTypes)
     return true;
 
   // 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()))
+  if (!VDType->isDependentType() && VD->hasAttr<AlignedAttr>() &&
+      Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VDType))
     return false;
 
   return true;

diff  --git a/clang/test/CXX/class/class.init/class.copy.elision/p3.cpp b/clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
index 29d818602537..e4056221b4f3 100644
--- a/clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
+++ b/clang/test/CXX/class/class.init/class.copy.elision/p3.cpp
@@ -292,3 +292,108 @@ NeedValue test_4_3() {
   return b; // cxx20-error {{calling a private constructor of class 'test_ctor_param_rvalue_ref::B2'}}
 }
 } // namespace test_ctor_param_rvalue_ref
+
+namespace test_lvalue_ref_is_not_moved_from {
+
+struct Target {};
+  // expected-note at -1 {{candidate constructor (the implicit copy constructor) not viable}}
+  // expected-note at -2 {{candidate constructor (the implicit move constructor) not viable}}
+  // cxx11_14_17-note at -3 {{candidate constructor (the implicit copy constructor) not viable}}
+  // cxx11_14_17-note at -4 {{candidate constructor (the implicit move constructor) not viable}}
+
+struct CopyOnly {
+  CopyOnly(CopyOnly&&) = delete; // cxx20-note {{has been explicitly marked deleted here}}
+  CopyOnly(CopyOnly&);
+  operator Target() && = delete; // cxx20-note {{has been explicitly marked deleted here}}
+  operator Target() &;
+};
+
+struct MoveOnly {
+  MoveOnly(MoveOnly&&); // expected-note {{copy constructor is implicitly deleted because}}
+    // cxx11_14_17-note at -1 {{copy constructor is implicitly deleted because}}
+  operator Target() &&; // expected-note {{candidate function not viable}}
+    // cxx11_14_17-note at -1 {{candidate function not viable}}
+};
+
+extern CopyOnly copyonly;
+extern MoveOnly moveonly;
+
+CopyOnly t1() {
+    CopyOnly& r = copyonly;
+    return r;
+}
+
+CopyOnly t2() {
+    CopyOnly&& r = static_cast<CopyOnly&&>(copyonly);
+    return r; // cxx20-error {{call to deleted constructor}}
+}
+
+MoveOnly t3() {
+    MoveOnly& r = moveonly;
+    return r; // expected-error {{call to implicitly-deleted copy constructor}}
+}
+
+MoveOnly t4() {
+    MoveOnly&& r = static_cast<MoveOnly&&>(moveonly);
+    return r; // cxx11_14_17-error {{call to implicitly-deleted copy constructor}}
+}
+
+Target t5() {
+    CopyOnly& r = copyonly;
+    return r;
+}
+
+Target t6() {
+    CopyOnly&& r = static_cast<CopyOnly&&>(copyonly);
+    return r; // cxx20-error {{invokes a deleted function}}
+}
+
+Target t7() {
+    MoveOnly& r = moveonly;
+    return r; // expected-error {{no viable conversion}}
+}
+
+Target t8() {
+    MoveOnly&& r = static_cast<MoveOnly&&>(moveonly);
+    return r; // cxx11_14_17-error {{no viable conversion}}
+}
+
+} // namespace test_lvalue_ref_is_not_moved_from
+
+namespace test_rvalue_ref_to_nonobject {
+
+struct CopyOnly {};
+struct MoveOnly {};
+
+struct Target {
+    Target(CopyOnly (&)());
+    Target(CopyOnly (&&)()) = delete;
+    Target(MoveOnly (&)()) = delete; // expected-note {{has been explicitly marked deleted here}}
+      // expected-note at -1 {{has been explicitly marked deleted here}}
+    Target(MoveOnly (&&)());
+};
+
+CopyOnly make_copyonly();
+MoveOnly make_moveonly();
+
+Target t1() {
+    CopyOnly (&r)() = make_copyonly;
+    return r;
+}
+
+Target t2() {
+    CopyOnly (&&r)() = static_cast<CopyOnly(&&)()>(make_copyonly);
+    return r; // OK in all modes; not subject to implicit move
+}
+
+Target t3() {
+    MoveOnly (&r)() = make_moveonly;
+    return r; // expected-error {{invokes a deleted function}}
+}
+
+Target t4() {
+    MoveOnly (&&r)() = static_cast<MoveOnly(&&)()>(make_moveonly);
+    return r; // expected-error {{invokes a deleted function}}
+}
+
+} // namespace test_rvalue_ref_to_nonobject