From afe2faf6edb18d6b622e84d4cd3aff938e638e3f Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Wed, 2 Oct 2019 02:32:15 +0000 Subject: [PATCH] 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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@373421 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Sema/SemaInit.cpp | 51 ++++++++--------- test/SemaCXX/warn-pessmizing-move.cpp | 80 +++++++++++++++------------ test/SemaCXX/warn-redundant-move.cpp | 14 ----- 3 files changed, 68 insertions(+), 77 deletions(-) diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp index e18ae74c25..9003a87cc8 100644 --- a/lib/Sema/SemaInit.cpp +++ b/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(InitExpr->IgnoreParens()); - if (!CCE || CCE->getNumArgs() != 1) - return; + unsigned DiagID = 0; + if (IsReturnStmt) { + const CXXConstructExpr *CCE = + dyn_cast(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(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(Arg)) - return; - - if (isa(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(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) { diff --git a/test/SemaCXX/warn-pessmizing-move.cpp b/test/SemaCXX/warn-pessmizing-move.cpp index deedec41bd..2c27cd7f95 100644 --- a/test/SemaCXX/warn-pessmizing-move.cpp +++ b/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 typename remove_reference::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(); } } - -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 e04a525641..2bfc8c9312 100644 --- a/test/SemaCXX/warn-redundant-move.cpp +++ b/test/SemaCXX/warn-redundant-move.cpp @@ -114,17 +114,3 @@ namespace templates { test2(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}:"" -} -- 2.40.0