diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h --- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h +++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONSTANTARGUMENTCHECK_H #include "../ClangTidyCheck.h" +#include "llvm/ADT/DenseSet.h" namespace clang { namespace tidy { @@ -36,6 +37,7 @@ private: const bool CheckTriviallyCopyableMove; + llvm::DenseSet AlreadyCheckedMoves; }; } // namespace performance diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp --- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp @@ -47,17 +47,62 @@ Finder->addMatcher(MoveCallMatcher, this); + auto ConstTypeParmMatcher = + qualType(references(isConstQualified())).bind("invocation-parm-type"); + auto RValueTypeParmMatcher = + qualType(rValueReferenceType()).bind("invocation-parm-type"); + // Matches respective ParmVarDecl for a CallExpr or CXXConstructExpr. + auto ArgumentWithParamMatcher = forEachArgumentWithParam( + MoveCallMatcher, parmVarDecl(anyOf(hasType(ConstTypeParmMatcher), + hasType(RValueTypeParmMatcher))) + .bind("invocation-parm")); + // Matches respective types of arguments for a CallExpr or CXXConstructExpr + // and it works on calls through function pointers as well. + auto ArgumentWithParamTypeMatcher = forEachArgumentWithParamType( + MoveCallMatcher, anyOf(ConstTypeParmMatcher, RValueTypeParmMatcher)); + Finder->addMatcher( - invocation(forEachArgumentWithParam( - MoveCallMatcher, - parmVarDecl(hasType(references(isConstQualified()))))) + invocation(anyOf(ArgumentWithParamMatcher, ArgumentWithParamTypeMatcher)) .bind("receiving-expr"), this); } +bool IsRValueReferenceParam(const Expr *Invocation, + const QualType &InvocationParmType, + const Expr *Arg) { + if (Invocation && InvocationParmType->isRValueReferenceType() && + Arg->isLValue()) { + if (!Invocation->getType()->isRecordType()) + return true; + else { + if (const auto *ConstructCallExpr = + dyn_cast(Invocation)) { + if (const auto *ConstructorDecl = ConstructCallExpr->getConstructor()) { + if (!ConstructorDecl->isCopyOrMoveConstructor() && + !ConstructorDecl->isDefaultConstructor()) + return true; + } + } + } + } + return false; +} + void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) { const auto *CallMove = Result.Nodes.getNodeAs("call-move"); const auto *ReceivingExpr = Result.Nodes.getNodeAs("receiving-expr"); + const auto *InvocationParm = + Result.Nodes.getNodeAs("invocation-parm"); + const auto *InvocationParmType = + Result.Nodes.getNodeAs("invocation-parm-type"); + + // Skipping matchers which have been matched. + if (!ReceivingExpr && AlreadyCheckedMoves.contains(CallMove)) + return; + + if (ReceivingExpr) + AlreadyCheckedMoves.insert(CallMove); + const Expr *Arg = CallMove->getArg(0); SourceManager &SM = Result.Context->getSourceManager(); @@ -90,20 +135,68 @@ return; bool IsVariable = isa(Arg); + // std::move shouldn't be removed when an lvalue wrapped by std::move is + // passed to the function with an rvalue reference parameter. + bool IsRVRefParam = + IsRValueReferenceParam(ReceivingExpr, *InvocationParmType, Arg); const auto *Var = IsVariable ? dyn_cast(Arg)->getDecl() : nullptr; - auto Diag = diag(FileMoveRange.getBegin(), - "std::move of the %select{|const }0" - "%select{expression|variable %4}1 " - "%select{|of the trivially-copyable type %5 }2" - "has no effect; remove std::move()" - "%select{| or make the variable non-const}3") - << IsConstArg << IsVariable << IsTriviallyCopyable - << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var - << Arg->getType(); - replaceCallWithArg(CallMove, Diag, SM, getLangOpts()); + { + auto Diag = diag(FileMoveRange.getBegin(), + "std::move of the %select{|const }0" + "%select{expression|variable %5}1 " + "%select{|of the trivially-copyable type %6 }2" + "has no effect%select{; remove std::move()|}3" + "%select{| or make the variable non-const}4") + << IsConstArg << IsVariable << IsTriviallyCopyable + << IsRVRefParam + << (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var + << Arg->getType(); + if (!IsRVRefParam) + replaceCallWithArg(CallMove, Diag, SM, getLangOpts()); + } + if (IsRVRefParam) { + // Generate notes for an invocation with an rvalue reference parameter. + const auto *ReceivingCallExpr = dyn_cast(ReceivingExpr); + const auto *ReceivingConstructExpr = + dyn_cast(ReceivingExpr); + // Skipping the invocation which is a template instantiation. + if ((!ReceivingCallExpr || !ReceivingCallExpr->getDirectCallee() || + ReceivingCallExpr->getDirectCallee()->isTemplateInstantiation()) && + (!ReceivingConstructExpr || + !ReceivingConstructExpr->getConstructor() || + ReceivingConstructExpr->getConstructor()->isTemplateInstantiation())) + return; + + const NamedDecl *FunctionName = nullptr; + FunctionName = + ReceivingCallExpr + ? ReceivingCallExpr->getDirectCallee()->getUnderlyingDecl() + : ReceivingConstructExpr->getConstructor()->getUnderlyingDecl(); + + QualType NoRefType = (*InvocationParmType)->getPointeeType(); + PrintingPolicy PolicyWithSuppressedTag(getLangOpts()); + PolicyWithSuppressedTag.SuppressTagKeyword = true; + PolicyWithSuppressedTag.SuppressUnwrittenScope = true; + std::string ExpectParmTypeName = + NoRefType.getAsString(PolicyWithSuppressedTag); + if (!NoRefType->isPointerType()) { + NoRefType.addConst(); + ExpectParmTypeName = + NoRefType.getAsString(PolicyWithSuppressedTag) + " &"; + } + + diag(InvocationParm->getLocation(), + "consider changing the %ordinal0 parameter of %1 from %2 to '%3'", + DiagnosticIDs::Note) + << (InvocationParm->getFunctionScopeIndex() + 1) << FunctionName + << *InvocationParmType << ExpectParmTypeName; + } } else if (ReceivingExpr) { + if ((*InvocationParmType)->isRValueReferenceType()) + return; + auto Diag = diag(FileMoveRange.getBegin(), "passing result of std::move() as a const reference " "argument; no move will actually happen"); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -175,6 +175,9 @@ option to control whether to warn on narrowing integer to floating-point conversions. +- Improved :doc:`performance-move-const-arg` check. + + Removed a wrong FixIt for trivially copyable objects wrapped by ``std::move()`` and passed to an rvalue reference parameter. Removal of ``std::move()`` would break the code. Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp @@ -246,3 +246,97 @@ }; f(MoveSemantics()); } + +void showInt(int &&v); +void showInt(int v1, int &&v2); +void showPointer(const char *&&s); +void showPointer2(const char *const &&s); +void showTriviallyCopyable(TriviallyCopyable &&obj); +void showTriviallyCopyablePointer(const TriviallyCopyable *&&obj); +void testFunctions() { + int a = 10; + showInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-10]]:20: note: consider changing the 1st parameter of 'showInt' from 'int &&' to 'const int &' + showInt(int()); + showInt(a, std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-13]]:28: note: consider changing the 2nd parameter of 'showInt' from 'int &&' to 'const int &' + const char* s = ""; + showPointer(std::move(s)); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *' + showPointer2(std::move(s)); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const' + TriviallyCopyable *obj = new TriviallyCopyable(); + showTriviallyCopyable(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-21]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + showTriviallyCopyablePointer(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *' +} +template +void forwardToShowInt(T && t) { + showInt(static_cast(t)); +} +void testTemplate() { + int a = 10; + forwardToShowInt(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] +} + +struct Tmp { + Tmp(); + Tmp(int &&a); + Tmp(int v1, int &&a); + Tmp(const char *&&s); + Tmp(TriviallyCopyable&& obj); + Tmp(const TriviallyCopyable *&&obj); + void showTmp(TriviallyCopyable&& t); + static void showTmpStatic(TriviallyCopyable&& t); +}; +void testMethods() { + Tmp t; + int a = 10; + Tmp t1(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &' + Tmp t2(a, std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &' + const char* s = ""; + Tmp t3(std::move(s)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *' + TriviallyCopyable *obj = new TriviallyCopyable(); + Tmp t4(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-21]]:27: note: consider changing the 1st parameter of 'Tmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + Tmp t5(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-23]]:34: note: consider changing the 1st parameter of 'Tmp' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *' + t.showTmp(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-25]]:36: note: consider changing the 1st parameter of 'showTmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + Tmp::showTmpStatic(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-27]]:49: note: consider changing the 1st parameter of 'showTmpStatic' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' +} + +void showA(A &&v) {} +void testA() { + A a; + showA(std::move(a)); +} + +void testFuncPointer() { + int a = 10; + void (*choice)(int, int &&); + choice = showInt; + choice(std::move(a), std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect; remove std::move() [performance-move-const-arg] + // CHECK-FIXES: choice(a, std::move(a)); + // CHECK-MESSAGES: :[[@LINE-3]]:24: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] +}