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

r368237 attempted to improve fix-its for move warnings, but introduced some regressions to -Wpessimizing-move. Revert that change and add the missing test cases to the pessimizing move test to prevent future regressions. llvm-svn: 373421
2019-10-02 02:32:15 +00:00 · 2019-10-02 02:32:15 +00:00 · e388725316
parent bfc68885d9
commit e388725316
3 changed files with 68 additions and 77 deletions
--- a/clang/lib/Sema/SemaInit.cpp
+++ b/clang/lib/Sema/SemaInit.cpp
@ -7580,34 +7580,27 @@ static void CheckMoveOnConstruction(Sema &S, const Expr *InitExpr,
  if (!DestType->isRecordType())
    return;

-  const CXXConstructExpr *CCE =
-      dyn_cast<CXXConstructExpr>(InitExpr->IgnoreParens());
-  if (!CCE || CCE->getNumArgs() != 1)
-    return;
+  unsigned DiagID = 0;
+  if (IsReturnStmt) {
+    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);
+  const Expr *Arg = CE->getArg(0)->IgnoreImplicit();

-  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
+  if (IsReturnStmt) {
    const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Arg->IgnoreParenImpCasts());
    if (!DRE || DRE->refersToEnclosingVariableOrCapture())
      return;
@ -7634,18 +7627,24 @@ 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 BeginLoc = CCE->getBeginLoc();
-  if (BeginLoc.isMacroID())
+  SourceLocation CallBegin = CE->getCallee()->getBeginLoc();
+  if (CallBegin.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
@ -7656,16 +7655,14 @@ static void CheckMoveOnConstruction(Sema &S, const Expr *InitExpr,
    ArgLoc = S.getSourceManager().getImmediateExpansionRange(ArgLoc).getBegin();
  }

-  SourceLocation LParen = ArgLoc.getLocWithOffset(-1);
  if (LParen.isMacroID())
    return;
-  SourceLocation EndLoc = CCE->getEndLoc();
-  if (EndLoc.isMacroID())
-    return;
+
+  LParen = ArgLoc.getLocWithOffset(-1);

  S.Diag(CE->getBeginLoc(), diag::note_remove_move)
-      << FixItHint::CreateRemoval(SourceRange(BeginLoc, LParen))
-      << FixItHint::CreateRemoval(SourceRange(RParen, EndLoc));
+      << FixItHint::CreateRemoval(SourceRange(CallBegin, LParen))
+      << FixItHint::CreateRemoval(SourceRange(RParen, RParen));
 }

 static void CheckForNullPointerDereference(Sema &S, const Expr *E) {
--- a/clang/test/SemaCXX/warn-pessmizing-move.cpp
+++ b/clang/test/SemaCXX/warn-pessmizing-move.cpp
@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wpessimizing-move -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wpessimizing-move -std=c++11 -verify %s -DUSER_DEFINED
 // RUN: %clang_cc1 -fsyntax-only -Wpessimizing-move -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s

 // definitions for std::move
@ -12,7 +13,15 @@ template <class T> typename remove_reference<T>::type &&move(T &&t);
 }
 }

-struct A {};
+struct A {
+#ifdef USER_DEFINED
+  A() {}
+  A(const A &) {}
+  A(A &&) {}
+  A &operator=(const A &) { return *this; }
+  A &operator=(A &&) { return *this; }
+#endif
+};
 struct B {
  B() {}
  B(A) {}
@ -47,6 +56,19 @@ B test2(A a1, B b1) {
  // 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 A();
+  return test1(a2);
+  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]]:23-[[@LINE-4]]:24}:""
+  return std::move(test1(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]]:29-[[@LINE-4]]:30}:""
 }

 A global_a;
@ -101,11 +123,24 @@ void test6() {
  // expected-note@-2{{remove std::move call}}
  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:20}:""
  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:24}:""
+
+  a3 = std::move(A());
+  // expected-warning@-1{{prevents copy elision}}
+  // expected-note@-2{{remove std::move call}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:8-[[@LINE-3]]:18}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:21-[[@LINE-4]]:22}:""
+
  A a4 = std::move(test3());
  // 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]]:27-[[@LINE-4]]:28}:""
+
+  a4 = std::move(test3());
+  // expected-warning@-1{{prevents copy elision}}
+  // expected-note@-2{{remove std::move call}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:8-[[@LINE-3]]:18}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:25-[[@LINE-4]]:26}:""
 }

 A test7() {
@ -122,13 +157,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]]:10-[[@LINE-3]]:21}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:24-[[@LINE-4]]:26}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:24-[[@LINE-4]]:25}:""
  A a4 = (std::move((A())));
  // expected-warning@-1{{prevents copy elision}}
  // expected-note@-2{{remove std::move call}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:10-[[@LINE-3]]:21}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:26-[[@LINE-4]]:28}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:26-[[@LINE-4]]:27}:""

  return std::move(a1);
  // expected-warning@-1{{prevents copy elision}}
@ -143,13 +178,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]]:10-[[@LINE-3]]:21}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:25}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:23-[[@LINE-4]]:24}:""
  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]]:21}:""
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:25-[[@LINE-4]]:27}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:21}:""
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:25-[[@LINE-4]]:26}:""
 }

 #define wrap1(x) x
@ -227,30 +262,3 @@ 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}:""
-}
--- a/clang/test/SemaCXX/warn-redundant-move.cpp
+++ b/clang/test/SemaCXX/warn-redundant-move.cpp
@ -114,17 +114,3 @@ 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}:""
-}