From: Richard Trieu <rtrieu@google.com>
Date: Thu, 8 Aug 2019 00:12:51 +0000 (+0000)
Subject: Update fix-it hints for std::move warnings.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=e74367a39b6d2bbdcb1a38c0fe6c58e214b97741;p=clang

Update fix-it hints for std::move warnings.

Fix -Wpessimizing-move and -Wredundant-move when warning on initializer lists.
The new fix-it hints for removing the std::move call will now also suggest
removing the braces for the initializer list so that the resulting code will
still be compilable.

This fixes PR42832


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@368237 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp
index 7fbffd956c..693d759db2 100644
--- a/lib/Sema/SemaInit.cpp
+++ b/lib/Sema/SemaInit.cpp
@@ -7305,27 +7305,34 @@ static void CheckMoveOnConstruction(Sema &S, const Expr *InitExpr,
   if (!DestType->isRecordType())
     return;
 
-  unsigned DiagID = 0;
-  if (IsReturnStmt) {
-    const CXXConstructExpr *CCE =
-        dyn_cast<CXXConstructExpr>(InitExpr->IgnoreParens());
-    if (!CCE || CCE->getNumArgs() != 1)
-      return;
+  const CXXConstructExpr *CCE =
+      dyn_cast<CXXConstructExpr>(InitExpr->IgnoreParens());
+  if (!CCE || CCE->getNumArgs() != 1)
+    return;
 
-    if (!CCE->getConstructor()->isCopyOrMoveConstructor())
-      return;
+  if (!CCE->getConstructor()->isCopyOrMoveConstructor())
+    return;
 
-    InitExpr = CCE->getArg(0)->IgnoreImpCasts();
-  }
+  InitExpr = CCE->getArg(0)->IgnoreImpCasts();
 
   // Find the std::move call and get the argument.
   const CallExpr *CE = dyn_cast<CallExpr>(InitExpr->IgnoreParens());
   if (!CE || !CE->isCallToStdMove())
     return;
 
-  const Expr *Arg = CE->getArg(0)->IgnoreImplicit();
+  const Expr *Arg = CE->getArg(0);
 
-  if (IsReturnStmt) {
+  unsigned DiagID = 0;
+
+  if (!IsReturnStmt && !isa<MaterializeTemporaryExpr>(Arg))
+    return;
+
+  if (isa<MaterializeTemporaryExpr>(Arg)) {
+    DiagID = diag::warn_pessimizing_move_on_initialization;
+    const Expr *ArgStripped = Arg->IgnoreImplicit()->IgnoreParens();
+    if (!ArgStripped->isRValue() || !ArgStripped->getType()->isRecordType())
+      return;
+  } else { // IsReturnStmt
     const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Arg->IgnoreParenImpCasts());
     if (!DRE || DRE->refersToEnclosingVariableOrCapture())
       return;
@@ -7352,24 +7359,18 @@ static void CheckMoveOnConstruction(Sema &S, const Expr *InitExpr,
       DiagID = 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->getBeginLoc(), DiagID);
 
   // Get all the locations for a fix-it.  Don't emit the fix-it if any location
   // is within a macro.
-  SourceLocation CallBegin = CE->getCallee()->getBeginLoc();
-  if (CallBegin.isMacroID())
+  SourceLocation BeginLoc = CCE->getBeginLoc();
+  if (BeginLoc.isMacroID())
     return;
   SourceLocation RParen = CE->getRParenLoc();
   if (RParen.isMacroID())
     return;
-  SourceLocation LParen;
   SourceLocation ArgLoc = Arg->getBeginLoc();
 
   // Special testing for the argument location.  Since the fix-it needs the
@@ -7380,14 +7381,16 @@ static void CheckMoveOnConstruction(Sema &S, const Expr *InitExpr,
     ArgLoc = S.getSourceManager().getImmediateExpansionRange(ArgLoc).getBegin();
   }
 
+  SourceLocation LParen = ArgLoc.getLocWithOffset(-1);
   if (LParen.isMacroID())
     return;
-
-  LParen = ArgLoc.getLocWithOffset(-1);
+  SourceLocation EndLoc = CCE->getEndLoc();
+  if (EndLoc.isMacroID())
+    return;
 
   S.Diag(CE->getBeginLoc(), diag::note_remove_move)
-      << FixItHint::CreateRemoval(SourceRange(CallBegin, LParen))
-      << FixItHint::CreateRemoval(SourceRange(RParen, RParen));
+      << FixItHint::CreateRemoval(SourceRange(BeginLoc, LParen))
+      << FixItHint::CreateRemoval(SourceRange(RParen, EndLoc));
 }
 
 static void CheckForNullPointerDereference(Sema &S, const Expr *E) {
diff --git a/test/SemaCXX/warn-pessmizing-move.cpp b/test/SemaCXX/warn-pessmizing-move.cpp
index 71e99dca00..deedec41bd 100644
--- a/test/SemaCXX/warn-pessmizing-move.cpp
+++ b/test/SemaCXX/warn-pessmizing-move.cpp
@@ -122,13 +122,13 @@ A test7() {
   A a3 = (std::move(A()));
   // expected-warning@-1{{prevents copy elision}}
   // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:24-[[@LINE-4]]:25}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:24-[[@LINE-4]]:26}:""
   A a4 = (std::move((A())));
   // expected-warning@-1{{prevents copy elision}}
   // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:26-[[@LINE-4]]:27}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:26-[[@LINE-4]]:28}:""
 
   return std::move(a1);
   // expected-warning@-1{{prevents copy elision}}
@@ -143,13 +143,13 @@ A test7() {
   return (std::move(a1));
   // expected-warning@-1{{prevents copy elision}}
   // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:24}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:25}:""
   return (std::move((a1)));
   // expected-warning@-1{{prevents copy elision}}
   // 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}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:25-[[@LINE-4]]:27}:""
 }
 
 #define wrap1(x) x
@@ -227,3 +227,30 @@ namespace templates {
     test2<B, A>();
   }
 }
+
+A init_list() {
+  A a1;
+  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]]:22}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:24-[[@LINE-4]]:27}:""
+
+  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]]:23}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:25-[[@LINE-4]]:29}:""
+
+  A a2 = { std::move(A()) };
+  // expected-warning@-1{{prevents copy elision}}
+  // expected-note@-2{{remove std::move call}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:22}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:25-[[@LINE-4]]:28}:""
+
+  A a3 = { (std::move(A())) };
+  // expected-warning@-1{{prevents copy elision}}
+  // expected-note@-2{{remove std::move call}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:23}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:26-[[@LINE-4]]:30}:""
+}
diff --git a/test/SemaCXX/warn-redundant-move.cpp b/test/SemaCXX/warn-redundant-move.cpp
index 2bfc8c9312..e04a525641 100644
--- a/test/SemaCXX/warn-redundant-move.cpp
+++ b/test/SemaCXX/warn-redundant-move.cpp
@@ -114,3 +114,17 @@ namespace templates {
     test2<B, A>(A());
   }
 }
+
+A init_list(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]]:10-[[@LINE-3]]:22}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:26}:""
+
+  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]]:10-[[@LINE-3]]:23}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:24-[[@LINE-4]]:28}:""
+}