[Sema] Emit correct warning when copy-elision is not possible.

[davide]

Summary

As Richard notes in PR23819, if we're returning a function parameter, copy elision isn't possible.

[rsmith]

lib/Sema/SemaInit.cpp

5991–6001

Can you just check isa<ParmVarDecl>(VD)?

[davide]

Indeed.

[rsmith]

LGTM

[rsmith]

test/SemaCXX/warn-pessmizing-move.cpp

158

Do we have more macro tests that test cases where the warning is produced? If not, please change the tests to use a local variable.

[davide]

test/SemaCXX/warn-pessmizing-move.cpp

158

No, we do not. Therefore I changed as you recommended. Thanks for noticing and the review in general.

lib/Sema/SemaInit.cpp

 
     const VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl());
     if (!VD || !VD->hasLocalStorage())
       return;
 
     if (!VD->getType()->isRecordType())
       return;
 
+    // If we're returning a function parameter, copy elision
+    // is not possible.
+    if (const FunctionDecl *FD =
+        dyn_cast_or_null<FunctionDecl>(VD->getParentFunctionOrMethod())) {
+      for (const auto *Param : FD->params()) {
+        if (Param == VD) {
+          DiagID = diag::warn_redundant_move_on_return;
+          break;
+        }
+      }
+    }
+
     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_initialization;
     const Expr *ArgStripped = Arg->IgnoreImplicit()->IgnoreParens();

test/SemaCXX/warn-pessmizing-move.cpp

struct B {
   B(A) {}
 };
 
 A test1(A a1) {
   A a2;
   return a1;
   return a2;
   return std::move(a1);
-  // expected-warning@-1{{prevents copy elision}}
-  // 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{{prevents copy elision}}
   // 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}:""
 }
 
 B test2(A a1, B b1) {
   // Object is different than return type so don't warn.
   A a2;
   return a1;
   return a2;
   return std::move(a1);
   return std::move(a2);
 
   B b2;
   return b1;
   return b2;
   return std::move(b1);
-  // expected-warning@-1{{prevents copy elision}}
-  // 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{{prevents copy elision}}
   // 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}:""
 }
 
 A global_a;
A test7() {
   // expected-note@-2{{remove std::move call}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
   // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:25-[[@LINE-4]]:26}:""
 }
 
 #define wrap1(x) x
 #define wrap2(x) x
 
-// Macro test.  Since the std::move call is outside the macro, it is
-// safe to suggest a fix-it.
+// Macro tests.
 A test8(A a) {
   return std::move(a);
-  // expected-warning@-1{{prevents copy elision}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:21-[[@LINE-4]]:22}:""
   return std::move(wrap1(a));
-  // expected-warning@-1{{prevents copy elision}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:28-[[@LINE-4]]:29}:""
   return std::move(wrap1(wrap2(a)));
-  // expected-warning@-1{{prevents copy elision}}
-  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:35-[[@LINE-4]]:36}:""
 }
 
 #define test9            \
   A test9(A a) {         \
     return std::move(a); \
   }
 
-// Macro test.  The std::call is inside the macro, so no fix-it is suggested.
 test9
-// expected-warning@-1{{prevents copy elision}}
-// CHECK-NOT: fix-it
 
 #define return_a return std::move(a)
 
 // Macro test.  The std::call is inside the macro, so no fix-it is suggested.
 A test10(A a) {
   return_a;
-  // expected-warning@-1{{prevents copy elision}}
-  // CHECK-NOT: fix-it
 }

test/SemaCXX/warn-redundant-move.cpp

C test4(A& a1, B& b1) {
   return b1;
   return b2;
 
   return std::move(a1);
   return std::move(a2);
   return std::move(b1);
   return std::move(b2);
 }
+
+//PR23819
+struct X {};
+X g();
+void h(X&&);
+X f(X x) { return std::move(x); } //expected-warning{{redundant move in return statement}} \
+                                  //expected-note{{remove std::move call here}}