Sockke created this revision.
Sockke added reviewers: alexfh, aaron.ballman, whisperity, steven.zhang, MTC.
Herald added subscribers: rnkovacs, xazax.hun.
Sockke requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
There are incorrect Fixit and missing warnings:
case 1:
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
}
case 2:
Using std::move() to wrap a pure rvalue or expiring value has no effect. Remove
std::move().
The object returned in the function does not need to be wrapped with std::move.
Because the returned nontrivially-copyable object will first call its own move
constructor.
struct TmpNR {
TmpNR() {}
TmpNR(const TmpNR&) {}
TmpNR(TmpNR&&) {}
};
void showTmpNR(TmpNR&&) {}
TmpNR testTmpNR() {
TmpNR tnr;
// expect: warning + fixit
TmpNR tnr2 = std::move(TmpNR()); // no warning <--- wrong diagnosis
// expect: warning + fixit
return std::move(tnr); // no warning <--- wrong diagnosis
}
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D107450
Files:
clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-move-const-arg.cpp
@@ -70,7 +70,11 @@
// CHECK-FIXES: return x3;
}
-A f4(A x4) { return std::move(x4); }
+A f4(A x4) {
+ return std::move(x4);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: passing result of std::move() as a const reference argument; no move will actually happen
+ // CHECK-FIXES: return x4;
+}
A f5(const A x5) {
return std::move(x5);
@@ -246,3 +250,73 @@
};
f(MoveSemantics());
}
+
+void showInt(int &&) {}
+int testInt() {
+ int a = 10;
+ int b = std::move(a);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect; remove std::move()
+ // CHECK-FIXES: int b = a;
+ 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; remove std::move()
+ return 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()
+ // CHECK-FIXES: return a;
+}
+
+struct Tmp {};
+void showTmp(Tmp &&) {}
+Tmp testTmp() {
+ Tmp t;
+ Tmp t1 = std::move(t);
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: std::move of the variable 't' of the trivially-copyable type 'Tmp' has no effect; remove std::move()
+ // CHECK-FIXES: Tmp t1 = t;
+ Tmp t2 = std::move(Tmp());
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: std::move of the expression of the trivially-copyable type 'Tmp' has no effect; remove std::move()
+ // CHECK-FIXES: Tmp t2 = Tmp();
+ showTmp(std::move(t));
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: std::move of the variable 't' of the trivially-copyable type 'Tmp' has no effect; remove std::move()
+ return std::move(t);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 't' of the trivially-copyable type 'Tmp' has no effect; remove std::move()
+ // CHECK-FIXES: return t;
+}
+
+struct Tmp_UnTriviallyC {
+ Tmp_UnTriviallyC() {}
+ Tmp_UnTriviallyC(const Tmp_UnTriviallyC &) {}
+};
+void showTmp_UnTriviallyC(Tmp_UnTriviallyC &&) {}
+Tmp_UnTriviallyC testTmp_UnTriviallyC() {
+ Tmp_UnTriviallyC tn;
+ Tmp_UnTriviallyC tn1 = std::move(tn);
+ // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: passing result of std::move() as a const reference argument; no move will actually happen
+ // CHECK-FIXES: Tmp_UnTriviallyC tn1 = tn;
+ Tmp_UnTriviallyC tn2 = std::move(Tmp_UnTriviallyC());
+ // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: passing result of std::move() as a const reference argument; no move will actually happen
+ // CHECK-FIXES: Tmp_UnTriviallyC tn2 = Tmp_UnTriviallyC();
+ showTmp_UnTriviallyC(std::move(tn));
+ // Expect no warning given here.
+ return std::move(tn);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: passing result of std::move() as a const reference argument; no move will actually happen
+ // CHECK-FIXES: return tn;
+}
+
+struct Tmp_UnTriviallyCR {
+ Tmp_UnTriviallyCR() {}
+ Tmp_UnTriviallyCR(const Tmp_UnTriviallyCR &) {}
+ Tmp_UnTriviallyCR(Tmp_UnTriviallyCR &&) {}
+};
+void showTmp_UnTriviallyCR(Tmp_UnTriviallyCR &&) {}
+Tmp_UnTriviallyCR testTmp_UnTriviallyCR() {
+ Tmp_UnTriviallyCR tnr;
+ Tmp_UnTriviallyCR tnr1 = std::move(tnr);
+ // Expect no warning given here.
+ Tmp_UnTriviallyCR tnr2 = std::move(Tmp_UnTriviallyCR());
+ // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: passing result of std::move() as a const reference argument; no move will actually happen
+ // CHECK-FIXES: Tmp_UnTriviallyCR tnr2 = Tmp_UnTriviallyCR();
+ showTmp_UnTriviallyCR(std::move(tnr));
+ // Expect no warning given here.
+ return std::move(tnr);
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: passing result of std::move() as a const reference argument; no move will actually happen
+ // CHECK-FIXES: return tnr;
+}
Index: clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
+++ clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp
@@ -45,19 +45,41 @@
unless(isInTemplateInstantiation()))
.bind("call-move");
- Finder->addMatcher(MoveCallMatcher, this);
+ Finder->addMatcher(
+ returnStmt(hasReturnValue(anyOf(
+ ignoringImplicit(ignoringParenCasts(MoveCallMatcher)),
+ cxxConstructExpr(
+ hasDeclaration(cxxConstructorDecl(
+ allOf(isUserProvided(), isMoveConstructor()))),
+ hasAnyArgument(ignoringImplicit(ignoringParenCasts(MoveCallMatcher)))))))
+ .bind("return-call-move"),
+ this);
+
+ Finder->addMatcher(
+ declStmt(hasSingleDecl(varDecl(hasInitializer(
+ ignoringImplicit(ignoringParenCasts(MoveCallMatcher))))))
+ .bind("decl-call-move"),
+ this);
Finder->addMatcher(
invocation(forEachArgumentWithParam(
MoveCallMatcher,
- parmVarDecl(hasType(references(isConstQualified())))))
+ parmVarDecl(anyOf(hasType(references(isConstQualified())),
+ hasType(rValueReferenceType())))
+ .bind("invocation-parm")))
.bind("receiving-expr"),
this);
}
void MoveConstArgCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
+ const auto *ReturnCallMove =
+ Result.Nodes.getNodeAs<ReturnStmt>("return-call-move");
+ const auto *DeclCallMove = Result.Nodes.getNodeAs<DeclStmt>("decl-call-move");
const auto *ReceivingExpr = Result.Nodes.getNodeAs<Expr>("receiving-expr");
+ const auto *InvocationParm =
+ Result.Nodes.getNodeAs<ParmVarDecl>("invocation-parm");
+
const Expr *Arg = CallMove->getArg(0);
SourceManager &SM = Result.Context->getSourceManager();
@@ -102,8 +124,18 @@
<< (IsConstArg && IsVariable && !IsTriviallyCopyable) << Var
<< Arg->getType();
+ if (ReceivingExpr &&
+ InvocationParm->getOriginalType()->isRValueReferenceType() &&
+ !ReceivingExpr->getType()->isRecordType() && Arg->isLValue())
+ return;
+
replaceCallWithArg(CallMove, Diag, SM, getLangOpts());
- } else if (ReceivingExpr) {
+ } else if (ReturnCallMove || DeclCallMove || ReceivingExpr) {
+ if (ReceivingExpr &&
+ InvocationParm->getOriginalType()->isRValueReferenceType() &&
+ Arg->isLValue())
+ return;
+
auto Diag = diag(FileMoveRange.getBegin(),
"passing result of std::move() as a const reference "
"argument; no move will actually happen");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
