Fix redundant move warning

[rtrieu]

Summary

Until DR1579 is implemented, redundant move should only be emitted when a parameter variable has the same type as the function return type and is returned. This removes the case where the returned type and the returned variable have different types. Also include a test to check for that the move constructor is used by checking the AST dump.

[rsmith]

LGTM

[rtrieu]

Committed at r243594.

lib/Sema/SemaInit.cpp

const CXXConstructExpr *CCE =
         dyn_cast<CXXConstructExpr>(InitExpr->IgnoreParens());
     if (!CCE || CCE->getNumArgs() != 1)
       return;
 
     if (!CCE->getConstructor()->isCopyOrMoveConstructor())
       return;
 
     InitExpr = CCE->getArg(0)->IgnoreImpCasts();
-
-    // Remove implicit temporary and constructor nodes.
-    if (const MaterializeTemporaryExpr *MTE =
-            dyn_cast<MaterializeTemporaryExpr>(InitExpr)) {
-      InitExpr = MTE->GetTemporaryExpr()->IgnoreImpCasts();
-      while (const CXXConstructExpr *CCE =
-                 dyn_cast<CXXConstructExpr>(InitExpr)) {
-        if (isa<CXXTemporaryObjectExpr>(CCE))
-          return;
-        if (CCE->getNumArgs() == 0)
-          return;
-        if (CCE->getNumArgs() > 1 && !isa<CXXDefaultArgExpr>(CCE->getArg(1)))
-          return;
-        InitExpr = CCE->getArg(0);
-      }
-      InitExpr = InitExpr->IgnoreImpCasts();
-      DiagID = diag::warn_redundant_move_on_return;
-    }
   }
 
   // Find the std::move call and get the argument.
   const CallExpr *CE = dyn_cast<CallExpr>(InitExpr->IgnoreParens());
   if (!CE || CE->getNumArgs() != 1)
     return;
 
   const FunctionDecl *MoveFunction = CE->getDirectCallee();
if (IsReturnStmt) {
     if (!VD || !VD->hasLocalStorage())
       return;
 
     QualType SourceType = VD->getType();
     if (!SourceType->isRecordType())
       return;
 
     if (!S.Context.hasSameUnqualifiedType(DestType, SourceType)) {
-      if (CXXRecordDecl *RD = SourceType->getAsCXXRecordDecl()) {
-        for (auto* Construct : RD->ctors()) {
-          if (Construct->isCopyConstructor() && Construct->isDeleted())
-            return;
-        }
-      }
-    }
 
     // If we're returning a function parameter, copy elision
     // is not possible.
     if (isa<ParmVarDecl>(VD))
       DiagID = diag::warn_redundant_move_on_return;
-
-    if (DiagID == 0) {
-      DiagID = S.Context.hasSameUnqualifiedType(DestType, VD->getType())
-                   ? diag::warn_pessimizing_move_on_return
-                   : diag::warn_redundant_move_on_return;
-    }
+    else
+      DiagID = diag::warn_pessimizing_move_on_return;
   } else {
     DiagID = diag::warn_pessimizing_move_on_initialization;
     const Expr *ArgStripped = Arg->IgnoreImplicit()->IgnoreParens();
     if (!ArgStripped->isRValue() || !ArgStripped->getType()->isRecordType())
       return;
   }
 
   S.Diag(CE->getLocStart(), DiagID);

test/SemaCXX/warn-pessmizing-move.cpp

A test10() {
   return_a;
   // expected-warning@-1{{prevents copy elision}}
   // CHECK-NOT: fix-it
 }
 
 namespace templates {
   struct A {};
   struct B { B(A); };
-  struct C { C(); C(C&&); };
-  struct D { D(C); };
 
   // Warn once here since the type is not dependent.
   template <typename T>
   A test1() {
     A a;
     return std::move(a);
     // expected-warning@-1{{prevents copy elision}}
     // expected-note@-2{{remove std::move call}}
     // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:22}:""
     // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:24}:""
   }
   void run_test1() {
     test1<A>();
     test1<B>();
-    test1<C>();
-    test1<D>();
   }
 
-  // Either a pessimizing move, a redundant move, or no warning could be
-  // emitted, given the right types.  So just drop the warning.
+  // T1 and T2 may not be the same, the warning may not always apply.
   template <typename T1, typename T2>
   T1 test2() {
     T2 t;
     return std::move(t);
   }
   void run_test2() {
     test2<A, A>();
     test2<B, A>();
-    test2<D, C>();
   }
 }

test/SemaCXX/warn-redundant-move.cpp

 // RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 -verify %s
-// RUN: not %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -fsyntax-only -Wredundant-move -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -ast-dump | FileCheck %s --check-prefix=CHECK-AST
 
 // definitions for std::move
 namespace std {
 inline namespace foo {
 template <class T> struct remove_reference { typedef T type; };
 template <class T> struct remove_reference<T&> { typedef T type; };
 template <class T> struct remove_reference<T&&> { typedef T type; };
 
 template <class T> typename remove_reference<T>::type &&move(T &&t);
 }
 }
 
+// test1 and test2 should not warn until after implementation of DR1579.
 struct A {};
 struct B : public A {};
 
 A test1(B b1) {
   B b2;
   return b1;
   return b2;
   return std::move(b1);
-  // expected-warning@-1{{redundant move}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:22-[[@LINE-4]]:23}:""
   return std::move(b2);
-  // expected-warning@-1{{redundant move}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:22-[[@LINE-4]]:23}:""
 }
 
 struct C {
   C() {}
   C(A) {}
 };
 
 C test2(A a1, B b1) {
   A a2;
   B b2;
 
   return a1;
   return a2;
   return b1;
   return b2;
 
   return std::move(a1);
-  // expected-warning@-1{{redundant move}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:22-[[@LINE-4]]:23}:""
   return std::move(a2);
-  // expected-warning@-1{{redundant move}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:22-[[@LINE-4]]:23}:""
   return std::move(b1);
-  // expected-warning@-1{{redundant move}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:22-[[@LINE-4]]:23}:""
   return std::move(b2);
-  // expected-warning@-1{{redundant move}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:22-[[@LINE-4]]:23}:""
 }
 
 // Copy of tests above with types changed to reference types.
 A test3(B& b1) {
   B& b2 = b1;
   return b1;
   return b2;
   return std::move(b1);
C test4(A& a1, B& b1) {
   return std::move(b1);
   return std::move(b2);
 }
 
 // PR23819, case 2
 struct D {};
 D test5(D d) {
   return d;
+  // Verify the implicit move from the AST dump
+  // CHECK-AST: ReturnStmt{{.*}}line:[[@LINE-2]]
+  // CHECK-AST-NEXT: CXXConstructExpr{{.*}}struct D{{.*}}void (struct D &&)
+  // CHECK-AST-NEXT: ImplicitCastExpr
+  // CHECK-AST-NEXT: DeclRefExpr{{.*}}ParmVar{{.*}}'d'
 
   return std::move(d);
   // expected-warning@-1{{redundant move in return statement}}
   // expected-note@-2{{remove std::move call here}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
   // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:21-[[@LINE-4]]:22}:""
 }
 
 namespace templates {
   struct A {};
   struct B { B(A); };
-  struct C { C(); C(C&&); };
-  struct D { D(C); };
 
   // Warn once here since the type is not dependent.
   template <typename T>
-  B test1() {
-    A a;
+  A test1(A a) {
     return std::move(a);
     // expected-warning@-1{{redundant move in return statement}}
     // expected-note@-2{{remove std::move call here}}
     // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:22}:""
     // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:24}:""
   }
   void run_test1() {
-    test1<A>();
-    test1<B>();
-    test1<C>();
-    test1<D>();
+    test1<A>(A());
+    test1<B>(A());
   }
 
-  // Either a pessimizing move, a redundant move, or no warning could be
-  // emitted, given the right types.  So just drop the warning.
+  // T1 and T2 may not be the same, the warning may not always apply.
   template <typename T1, typename T2>
-  T1 test2() {
-    T2 t;
+  T1 test2(T2 t) {
     return std::move(t);
   }
   void run_test2() {
-    test2<A, A>();
-    test2<B, A>();
-    test2<D, C>();
-  }
+    test2<A, A>(A());
+    test2<B, A>(A());
   }
-
-// No more fix-its past here.
-// CHECK-NOT: fix-it
-
-// A deleted copy constructor will prevent moves without std::move
-struct E {
-  E(E &&e);
-  E(const E &e) = delete;
-  // expected-note@-1{{deleted here}}
-};
-
-struct F {
-  F(E);
-  // expected-note@-1{{passing argument to parameter here}}
-};
-
-F test6(E e) {
-  return e;
-  // expected-error@-1{{call to deleted constructor of 'E'}}
-  return std::move(e);
-}
-
-struct G {
-  G(G &&g);
-  // expected-note@-1{{copy constructor is implicitly deleted because 'G' has a user-declared move constructor}}
-};
-
-struct H {
-  H(G);
-  // expected-note@-1{{passing argument to parameter here}}
-};
-
-H test6(G g) {
-  return g;  // expected-error{{call to implicitly-deleted copy constructor of 'G'}}
-  return std::move(g);
 }