Author: rtrieu Date: Tue Oct 1 19:32:15 2019 New Revision: 373421 URL: http://llvm.org/viewvc/llvm-project?rev=373421&view=rev Log: 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. Modified: cfe/trunk/lib/Sema/SemaInit.cpp cfe/trunk/test/SemaCXX/warn-pessmizing-move.cpp cfe/trunk/test/SemaCXX/warn-redundant-move.cpp Modified: cfe/trunk/lib/Sema/SemaInit.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=373421&r1=373420&r2=373421&view=diff ============================================================================== --- cfe/trunk/lib/Sema/SemaInit.cpp (original) +++ cfe/trunk/lib/Sema/SemaInit.cpp Tue Oct 1 19:32:15 2019 @@ -7580,34 +7580,27 @@ static void CheckMoveOnConstruction(Sema 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); - - unsigned DiagID = 0; - - if (!IsReturnStmt && !isa<MaterializeTemporaryExpr>(Arg)) - return; + const Expr *Arg = CE->getArg(0)->IgnoreImplicit(); - 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 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 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) { Modified: cfe/trunk/test/SemaCXX/warn-pessmizing-move.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-pessmizing-move.cpp?rev=373421&r1=373420&r2=373421&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/warn-pessmizing-move.cpp (original) +++ cfe/trunk/test/SemaCXX/warn-pessmizing-move.cpp Tue Oct 1 19:32:15 2019 @@ -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_refer } } -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}:"" -} Modified: cfe/trunk/test/SemaCXX/warn-redundant-move.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-redundant-move.cpp?rev=373421&r1=373420&r2=373421&view=diff ============================================================================== --- cfe/trunk/test/SemaCXX/warn-redundant-move.cpp (original) +++ cfe/trunk/test/SemaCXX/warn-redundant-move.cpp Tue Oct 1 19:32:15 2019 @@ -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}:"" -} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits