malcolm.parsons updated this revision to Diff 82443.
malcolm.parsons added a comment.
Add double backticks.
https://reviews.llvm.org/D28022
Files:
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tidy/utils/DeclRefExprUtils.cpp
clang-tidy/utils/DeclRefExprUtils.h
test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -82,6 +82,7 @@
struct PositiveConstValueConstructor {
PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
// CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
+ // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
};
template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
@@ -143,8 +144,29 @@
Obj.nonConstMethod();
}
-struct NegativeValueCopyConstructor {
- NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {}
+struct PositiveValueUnusedConstructor {
+ PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {}
+};
+
+struct PositiveValueCopiedConstructor {
+ PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {}
+ ExpensiveToCopyType Field;
+};
+
+struct PositiveValueMovableConstructor {
+ PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:70: warning: parameter 'Copy'
+ // CHECK-FIXES: PositiveValueMovableConstructor(ExpensiveMovableType Copy) : Field(std::move(Copy)) {}
+ ExpensiveMovableType Field;
+};
+
+struct NegativeValueMovedConstructor {
+ NegativeValueMovedConstructor(ExpensiveMovableType Copy) : Field(static_cast<ExpensiveMovableType &&>(Copy)) {}
+ ExpensiveMovableType Field;
};
template <typename T>
Index: test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
+++ test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
@@ -64,6 +64,7 @@
struct PositiveConstValueConstructor {
PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
// CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
+ // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
};
template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
@@ -125,8 +126,17 @@
Obj.nonConstMethod();
}
-struct NegativeValueCopyConstructor {
- NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {}
+struct PositiveValueUnusedConstructor {
+ PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueUnusedConstructor(const ExpensiveToCopyType& Copy) {}
+};
+
+struct PositiveValueCopiedConstructor {
+ PositiveValueCopiedConstructor(ExpensiveToCopyType Copy) : Field(Copy) {}
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'
+ // CHECK-FIXES: PositiveValueCopiedConstructor(const ExpensiveToCopyType& Copy) : Field(Copy) {}
+ ExpensiveToCopyType Field;
};
template <typename T>
Index: clang-tidy/utils/DeclRefExprUtils.h
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -28,23 +28,34 @@
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context);
-// Returns set of all DeclRefExprs to VarDecl in Stmt.
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
allDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt, ASTContext &Context);
-// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in
-// a const fashion.
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl``.
+llvm::SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context);
+
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` where
+/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
llvm::SmallPtrSet<const DeclRefExpr *, 16>
constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
ASTContext &Context);
-// Returns true if DeclRefExpr is the argument of a copy-constructor call expr.
-bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl`` where
+/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
+llvm::SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl,
+ ASTContext &Context);
+
+/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-constructor
+/// call expression within ``Decl``.
+bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context);
-// Returns true if DeclRefExpr is the argument of a copy-assignment operator
-// call expr.
-bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+/// Returns ``true`` if ``DeclRefExpr`` is the argument of a copy-assignment
+/// operator CallExpr within ``Decl``.
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context);
} // namespace decl_ref_expr
Index: clang-tidy/utils/DeclRefExprUtils.cpp
===================================================================
--- clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tidy/utils/DeclRefExprUtils.cpp
@@ -71,6 +71,40 @@
return DeclRefs;
}
+// Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
+// is the a const reference or value argument to a CallExpr or CXXConstructExpr.
+SmallPtrSet<const DeclRefExpr *, 16>
+constReferenceDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl,
+ ASTContext &Context) {
+ auto DeclRefToVar =
+ declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
+ auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
+ // Match method call expressions where the variable is referenced as the this
+ // implicit object argument and opertor call expression for member operators
+ // where the variable is the 0-th argument.
+ auto Matches =
+ match(decl(forEachDescendant(expr(
+ anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
+ cxxOperatorCallExpr(ConstMethodCallee,
+ hasArgument(0, DeclRefToVar)))))),
+ Decl, Context);
+ SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ auto ConstReferenceOrValue =
+ qualType(anyOf(referenceType(pointee(qualType(isConstQualified()))),
+ unless(anyOf(referenceType(), pointerType()))));
+ auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
+ DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+ Matches = match(decl(forEachDescendant(callExpr(UsedAsConstRefOrValueArg))),
+ Decl, Context);
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ Matches =
+ match(decl(forEachDescendant(cxxConstructExpr(UsedAsConstRefOrValueArg))),
+ Decl, Context);
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ return DeclRefs;
+}
+
bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
ASTContext &Context) {
// Collect all DeclRefExprs to the loop variable and all CallExprs and
@@ -93,30 +127,41 @@
return DeclRefs;
}
-bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+SmallPtrSet<const DeclRefExpr *, 16>
+allDeclRefExprs(const VarDecl &VarDecl, const Decl &Decl, ASTContext &Context) {
+ auto Matches = match(
+ decl(forEachDescendant(
+ declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef"))),
+ Decl, Context);
+ SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
+ extractNodesByIdTo(Matches, "declRef", DeclRefs);
+ return DeclRefs;
+}
+
+bool isCopyConstructorArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context) {
auto UsedAsConstRefArg = forEachArgumentWithParam(
declRefExpr(equalsNode(&DeclRef)),
parmVarDecl(hasType(matchers::isReferenceToConst())));
auto Matches = match(
- stmt(hasDescendant(
+ decl(hasDescendant(
cxxConstructExpr(UsedAsConstRefArg, hasDeclaration(cxxConstructorDecl(
isCopyConstructor())))
.bind("constructExpr"))),
- Stmt, Context);
+ Decl, Context);
return !Matches.empty();
}
-bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool isCopyAssignmentArgument(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context) {
auto UsedAsConstRefArg = forEachArgumentWithParam(
declRefExpr(equalsNode(&DeclRef)),
parmVarDecl(hasType(matchers::isReferenceToConst())));
auto Matches = match(
- stmt(hasDescendant(
+ decl(hasDescendant(
cxxOperatorCallExpr(UsedAsConstRefArg, hasOverloadedOperatorName("="))
.bind("operatorCallExpr"))),
- Stmt, Context);
+ Decl, Context);
return !Matches.empty();
}
Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===================================================================
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -47,14 +47,14 @@
return !Matches.empty();
}
-bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Stmt &Stmt,
+bool hasLoopStmtAncestor(const DeclRefExpr &DeclRef, const Decl &Decl,
ASTContext &Context) {
auto Matches =
- match(findAll(declRefExpr(
+ match(decl(forEachDescendant(declRefExpr(
equalsNode(&DeclRef),
unless(hasAncestor(stmt(anyOf(forStmt(), cxxForRangeStmt(),
- whileStmt(), doStmt())))))),
- Stmt, Context);
+ whileStmt(), doStmt()))))))),
+ Decl, Context);
return Matches.empty();
}
@@ -72,7 +72,7 @@
unless(referenceType())))),
decl().bind("param"));
Finder->addMatcher(
- functionDecl(isDefinition(),
+ functionDecl(hasBody(stmt()), isDefinition(),
unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
unless(isInstantiated()),
has(typeLoc(forEach(ExpensiveValueParamDecl))),
@@ -89,44 +89,33 @@
bool IsConstQualified =
Param->getType().getCanonicalType().isConstQualified();
- // Skip declarations delayed by late template parsing without a body.
- if (!Function->getBody())
- return;
-
- // Do not trigger on non-const value parameters when:
- // 1. they are in a constructor definition since they can likely trigger
- // modernize-pass-by-value which will suggest to move the argument.
- if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) ||
- !Function->doesThisDeclarationHaveABody()))
- return;
-
auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
- *Param, *Function->getBody(), *Result.Context);
+ *Param, *Function, *Result.Context);
auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
- *Param, *Function->getBody(), *Result.Context);
- // 2. they are not only used as const.
+ *Param, *Function, *Result.Context);
+
+ // Do not trigger on non-const value parameters when they are not only used as
+ // const.
if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
return;
// If the parameter is non-const, check if it has a move constructor and is
// only referenced once to copy-construct another object or whether it has a
// move assignment operator and is only referenced once when copy-assigned.
// In this case wrap DeclRefExpr with std::move() to avoid the unnecessary
// copy.
- if (!IsConstQualified) {
+ if (!IsConstQualified && AllDeclRefExprs.size() == 1) {
auto CanonicalType = Param->getType().getCanonicalType();
- if (AllDeclRefExprs.size() == 1 &&
- !hasLoopStmtAncestor(**AllDeclRefExprs.begin(), *Function->getBody(),
- *Result.Context) &&
+ const auto &DeclRefExpr = **AllDeclRefExprs.begin();
+
+ if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
utils::decl_ref_expr::isCopyConstructorArgument(
- **AllDeclRefExprs.begin(), *Function->getBody(),
- *Result.Context)) ||
+ DeclRefExpr, *Function, *Result.Context)) ||
(utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
utils::decl_ref_expr::isCopyAssignmentArgument(
- **AllDeclRefExprs.begin(), *Function->getBody(),
- *Result.Context)))) {
- handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context);
+ DeclRefExpr, *Function, *Result.Context)))) {
+ handleMoveFix(*Param, DeclRefExpr, *Result.Context);
return;
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
