[clang-tools-extra] a7f8aea - [clang-tidy] Fix wrong FixIt in performance-move-const-arg
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 20 22:31:03 PST 2022
Author: Sockke
Date: 2022-01-21T14:23:52+08:00
New Revision: a7f8aea71485c00db2ffb6288ae58475869048b1
URL: https://github.com/llvm/llvm-project/commit/a7f8aea71485c00db2ffb6288ae58475869048b1
DIFF: https://github.com/llvm/llvm-project/commit/a7f8aea71485c00db2ffb6288ae58475869048b1.diff
LOG: [clang-tidy] Fix wrong FixIt in performance-move-const-arg
There are incorrect Fixit and missing warnings:
case :
A trivially-copyable object wrapped by std::move is passed to the function with rvalue reference parameters. Removing std::move will cause compilation errors.
```
void showInt(int&&) {}
void testInt() {
int a = 10;
// expect: warning + nofix
showInt(std::move(a)); // showInt(a) <--- wrong fix
}
struct Tmp {};
void showTmp(Tmp&&) {}
void testTmp() {
Tmp t;
// expect: warning + nofix
showTmp(std::move(t)); // showTmp(t) <--- wrong fix
}
```
Reviewed By: aaron.ballman, Quuxplusone
Differential Revision: https://reviews.llvm.org/D107450
Added:
Modified:
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
index e946a1f39fe98..0e91451211aed 100644
--- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
@@ -47,17 +47,62 @@ void MoveConstArgCheck::registerMatchers(MatchFinder *Finder) {
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<CXXConstructExpr>(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<CallExpr>("call-move");
const auto *ReceivingExpr = Result.Nodes.getNodeAs<Expr>("receiving-expr");
+ const auto *InvocationParm =
+ Result.Nodes.getNodeAs<ParmVarDecl>("invocation-parm");
+ const auto *InvocationParmType =
+ Result.Nodes.getNodeAs<QualType>("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 @@ void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
return;
bool IsVariable = isa<DeclRefExpr>(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<DeclRefExpr>(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<CallExpr>(ReceivingExpr);
+ const auto *ReceivingConstructExpr =
+ dyn_cast<CXXConstructExpr>(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/clang-tidy/performance/MoveConstArgCheck.h b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.h
index 28fe8d523d943..4a93e4c306e30 100644
--- 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 @@ class MoveConstArgCheck : public ClangTidyCheck {
private:
const bool CheckTriviallyCopyableMove;
+ llvm::DenseSet<const CallExpr *> AlreadyCheckedMoves;
};
} // namespace performance
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 683e914b7e863..e3c1c4b9411bb 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -175,6 +175,9 @@ Changes in existing checks
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
index 06ed6e0b56b1c..c1e5761c538e9 100644
--- 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 @@ void lambda2() {
};
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 <class T>
+void forwardToShowInt(T && t) {
+ showInt(static_cast<T &&>(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]
+}
More information about the cfe-commits
mailing list