Index: clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp @@ -17,30 +17,62 @@ namespace tidy { namespace misc { +static void ReplaceCallWithArg(const CallExpr *Call, DiagnosticBuilder &Diag, + const SourceManager &SM, + const LangOptions &LangOpts) { + const Expr *Arg = Call->getArg(0); + + CharSourceRange BeforeArgumentsRange = Lexer::makeFileCharRange( + CharSourceRange::getCharRange(Call->getLocStart(), Arg->getLocStart()), + SM, LangOpts); + CharSourceRange AfterArgumentsRange = Lexer::makeFileCharRange( + CharSourceRange::getCharRange(Call->getLocEnd(), + Call->getLocEnd().getLocWithOffset(1)), + SM, LangOpts); + + if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) { + Diag << FixItHint::CreateRemoval(BeforeArgumentsRange) + << FixItHint::CreateRemoval(AfterArgumentsRange); + } +} + void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) { if (!getLangOpts().CPlusPlus) return; - Finder->addMatcher(callExpr(callee(functionDecl(hasName("::std::move"))), - argumentCountIs(1), - unless(isInTemplateInstantiation())) - .bind("call-move"), + + auto MoveCallMatcher = + callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1), + unless(isInTemplateInstantiation())) + .bind("call-move"); + + Finder->addMatcher(MoveCallMatcher, this); + + auto ConstParamMatcher = forEachArgumentWithParam( + MoveCallMatcher, parmVarDecl(hasType(references(isConstQualified())))); + + Finder->addMatcher(callExpr(ConstParamMatcher).bind("receiving-expr"), this); + Finder->addMatcher(cxxConstructExpr(ConstParamMatcher).bind("receiving-expr"), this); } void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) { const auto *CallMove = Result.Nodes.getNodeAs("call-move"); + const auto *ReceivingExpr = Result.Nodes.getNodeAs("receiving-expr"); const Expr *Arg = CallMove->getArg(0); SourceManager &SM = Result.Context->getSourceManager(); + CharSourceRange MoveRange = + CharSourceRange::getCharRange(CallMove->getSourceRange()); + CharSourceRange FileMoveRange = + Lexer::makeFileCharRange(MoveRange, SM, getLangOpts()); + if (!FileMoveRange.isValid()) + return; + bool IsConstArg = Arg->getType().isConstQualified(); bool IsTriviallyCopyable = Arg->getType().isTriviallyCopyableType(*Result.Context); if (IsConstArg || IsTriviallyCopyable) { - auto MoveRange = CharSourceRange::getCharRange(CallMove->getSourceRange()); - auto FileMoveRange = Lexer::makeFileCharRange(MoveRange, SM, getLangOpts()); - if (!FileMoveRange.isValid()) - return; bool IsVariable = isa(Arg); auto Diag = diag(FileMoveRange.getBegin(), "std::move of the %select{|const }0" @@ -49,19 +81,13 @@ "has no effect; remove std::move()") << IsConstArg << IsVariable << IsTriviallyCopyable; - auto BeforeArgumentsRange = Lexer::makeFileCharRange( - CharSourceRange::getCharRange(CallMove->getLocStart(), - Arg->getLocStart()), - SM, getLangOpts()); - auto AfterArgumentsRange = Lexer::makeFileCharRange( - CharSourceRange::getCharRange( - CallMove->getLocEnd(), CallMove->getLocEnd().getLocWithOffset(1)), - SM, getLangOpts()); - - if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) { - Diag << FixItHint::CreateRemoval(BeforeArgumentsRange) - << FixItHint::CreateRemoval(AfterArgumentsRange); - } + ReplaceCallWithArg(CallMove, Diag, SM, getLangOpts()); + } else if (ReceivingExpr) { + auto Diag = diag(FileMoveRange.getBegin(), + "passing result of std::move() as a const reference " + "argument; no move will actually happen"); + + ReplaceCallWithArg(CallMove, Diag, SM, getLangOpts()); } } Index: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst =================================================================== --- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst +++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst @@ -3,8 +3,17 @@ misc-move-const-arg =================== -The check warns if ``std::move()`` is called with a constant argument or an -argument of a trivially-copyable type, e.g.: +The check warns + + - if ``std::move()`` is called with a constant argument, + - if ``std::move()`` is called with an argument of a trivially-copyable type, + or + - if the result of ``std::move()`` is passed as a const reference argument. + +In all three cases, the check will suggest a fix that removes the +``std::move()``. + +Here are examples of each of the three cases: .. code:: c++ @@ -13,3 +22,7 @@ int x; return std::move(x); // Warning: std::move of the variable of a trivially-copyable type has no effect + + void f(const string &s); + string s; + f(std::move(s)); // Warning: passing result of std::move as a const reference argument; no move will actually happen Index: clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp +++ clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp @@ -71,3 +71,90 @@ f10(1); f10(1); } + +class NoMoveSemantics { + public: + NoMoveSemantics(); + NoMoveSemantics(const NoMoveSemantics &); + + NoMoveSemantics &operator=(const NoMoveSemantics &); +}; + +void callByConstRef(const NoMoveSemantics &); +void callByConstRef(int i, const NoMoveSemantics &); + +void moveToConstReferencePositives() { + NoMoveSemantics obj; + + // Basic case. + callByConstRef(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as + // CHECK-FIXES: callByConstRef(obj); + + // Also works for second argument. + callByConstRef(1, std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: passing result of std::move() as + // CHECK-FIXES: callByConstRef(1, obj); + + // Works if std::move() applied to a temporary. + callByConstRef(std::move(NoMoveSemantics())); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as + // CHECK-FIXES: callByConstRef(NoMoveSemantics()); + + // Works if calling a copy constructor. + NoMoveSemantics other(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: passing result of std::move() as + // CHECK-FIXES: NoMoveSemantics other(obj); + + // Works if calling assignment operator. + other = std::move(obj); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: passing result of std::move() as + // CHECK-FIXES: other = obj; +} + +class MoveSemantics { + public: + MoveSemantics(); + MoveSemantics(MoveSemantics &&); + + MoveSemantics &operator=(MoveSemantics &&); +}; + +void callByValue(MoveSemantics); + +void callByRValueRef(MoveSemantics &&); + +template +void templateFunction(T obj) { + T other = std::move(obj); +} + +#define M3(T, obj) \ + do { \ + T other = std::move(obj); \ + } while (true) + +#define CALL(func) (func)() + +void moveToConstReferenceNegatives() { + // No warning when actual move takes place. + MoveSemantics move_semantics; + callByValue(std::move(move_semantics)); + callByRValueRef(std::move(move_semantics)); + MoveSemantics other(std::move(move_semantics)); + other = std::move(move_semantics); + + // No warning if std::move() not used. + NoMoveSemantics no_move_semantics; + callByConstRef(no_move_semantics); + + // No warning if instantiating a template. + templateFunction(no_move_semantics); + + // No warning inside of macro expansions. + M3(NoMoveSemantics, no_move_semantics); + + // No warning inside of macro expansion, even if the macro expansion is inside + // a lambda that is, in turn, an argument to a macro. + CALL([no_move_semantics] { M3(NoMoveSemantics, no_move_semantics); }); +}