[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
This revision was automatically updated to reflect the committed changes.
Closed by commit rL290883: [clang-tidy] Handle constructors in
performance-unnecessary-value-param (authored by malcolm.parsons).
Changed prior to commit:
https://reviews.llvm.org/D28022?vs=82443=82865#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28022
Files:
clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp
clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.h
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
clang-tools-extra/trunk/test/clang-tidy/performance-unnecessary-value-param.cpp
Index: clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -47,14 +47,14 @@
return !Matches.empty();
}
-bool hasLoopStmtAncestor(const DeclRefExpr , const Stmt ,
+bool hasLoopStmtAncestor(const DeclRefExpr , const Decl ,
ASTContext ) {
auto Matches =
- match(findAll(declRefExpr(
+ match(decl(forEachDescendant(declRefExpr(
equalsNode(),
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(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 = **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;
}
}
Index: clang-tools-extra/trunk/clang-tidy/utils/DeclRefExprUtils.cpp
===
---
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
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 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(Copy)) {}
+ ExpensiveMovableType Field;
};
template
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 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
Index: clang-tidy/utils/DeclRefExprUtils.h
===
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -28,23 +28,34 @@
bool isOnlyUsedAsConst(const VarDecl , const Stmt ,
ASTContext );
-// Returns set of all DeclRefExprs to VarDecl in Stmt.
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt``.
llvm::SmallPtrSet
allDeclRefExprs(const VarDecl , const Stmt , ASTContext );
-// 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
+allDeclRefExprs(const VarDecl , const Decl , ASTContext );
+
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Stmt`` where
+/// ``VarDecl`` is guaranteed to be accessed in a const fashion.
llvm::SmallPtrSet
constReferenceDeclRefExprs(const VarDecl , const Stmt ,
ASTContext );
-// Returns true if DeclRefExpr is the argument of a copy-constructor call expr.
-bool isCopyConstructorArgument(const DeclRefExpr , const Stmt ,
+/// Returns set of all ``DeclRefExprs`` to ``VarDecl`` within ``Decl`` where
+///
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
aaron.ballman added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); alexfh wrote: > aaron.ballman wrote: > > malcolm.parsons wrote: > > > aaron.ballman wrote: > > > > malcolm.parsons wrote: > > > > > malcolm.parsons wrote: > > > > > > aaron.ballman wrote: > > > > > > > Instead of using `hasBody()` and `getDefinition()`, you should > > > > > > > use `FunctionDecl::getBody()` and pass in an argument to receive > > > > > > > the function's definition. > > > > > > I don't want the body - the `CXXCtorInitializer`s are not in it. > > > > > I should use `hasBody()` and pass in an argument. > > > > You are calling `Function-hasBody()` followed by > > > > `Function->getDefinition()`, which is what `FunctionDecl::getBody()` > > > > does. The difference is that `FunctionDecl::getBody()` will also load > > > > serialized bodies from the AST (if the body happens to be in a PCH, for > > > > instance). > > > Do I want to load serialized bodies? > > I would imagine you would want to deserialize, since you care about the > > contents of the body, not just the presence of one. This may or may not be > > important with modules (Richard Smith would be able to give a more > > definitive answer there). > I would suggest writing minimal sane code that works until we have good > reasons to write more complex code to support module-enabled builds (and > appropriate tests). I think either approach is minimal, sane code and we should understand why we shouldn't use the interface provided by `FunctionDecl::getBody()` for getting the definition before deciding to not use it. I *think* we're fine to not use `getBody()` because the matcher should match over all of the function declarations with a definition, and so eventually it will find the definition with the actual body. In the case that the function definition is serialized, I think this already does the right thing and deserializes it. (I think the only case where we need the deserialization may be when we have a function declaration and we need to traverse from that to the function body, which may be in a different `FunctionDecl`.) So the use of `hasBody()` in the matcher above seems like the correct solution to me. https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
alexfh added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); aaron.ballman wrote: > malcolm.parsons wrote: > > aaron.ballman wrote: > > > malcolm.parsons wrote: > > > > malcolm.parsons wrote: > > > > > aaron.ballman wrote: > > > > > > Instead of using `hasBody()` and `getDefinition()`, you should use > > > > > > `FunctionDecl::getBody()` and pass in an argument to receive the > > > > > > function's definition. > > > > > I don't want the body - the `CXXCtorInitializer`s are not in it. > > > > I should use `hasBody()` and pass in an argument. > > > You are calling `Function-hasBody()` followed by > > > `Function->getDefinition()`, which is what `FunctionDecl::getBody()` > > > does. The difference is that `FunctionDecl::getBody()` will also load > > > serialized bodies from the AST (if the body happens to be in a PCH, for > > > instance). > > Do I want to load serialized bodies? > I would imagine you would want to deserialize, since you care about the > contents of the body, not just the presence of one. This may or may not be > important with modules (Richard Smith would be able to give a more definitive > answer there). I would suggest writing minimal sane code that works until we have good reasons to write more complex code to support module-enabled builds (and appropriate tests). Comment at: clang-tidy/utils/DeclRefExprUtils.h:51 + +/// Returns true if DeclRefExpr is the argument of a copy-constructor call +/// expression within Decl. Please enclose `true`, `DeclRefExpr` and other inline code snippets in double backquotes. https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
aaron.ballman added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); malcolm.parsons wrote: > aaron.ballman wrote: > > malcolm.parsons wrote: > > > malcolm.parsons wrote: > > > > aaron.ballman wrote: > > > > > Instead of using `hasBody()` and `getDefinition()`, you should use > > > > > `FunctionDecl::getBody()` and pass in an argument to receive the > > > > > function's definition. > > > > I don't want the body - the `CXXCtorInitializer`s are not in it. > > > I should use `hasBody()` and pass in an argument. > > You are calling `Function-hasBody()` followed by > > `Function->getDefinition()`, which is what `FunctionDecl::getBody()` does. > > The difference is that `FunctionDecl::getBody()` will also load serialized > > bodies from the AST (if the body happens to be in a PCH, for instance). > Do I want to load serialized bodies? I would imagine you would want to deserialize, since you care about the contents of the body, not just the presence of one. This may or may not be important with modules (Richard Smith would be able to give a more definitive answer there). https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
malcolm.parsons added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); aaron.ballman wrote: > malcolm.parsons wrote: > > malcolm.parsons wrote: > > > aaron.ballman wrote: > > > > Instead of using `hasBody()` and `getDefinition()`, you should use > > > > `FunctionDecl::getBody()` and pass in an argument to receive the > > > > function's definition. > > > I don't want the body - the `CXXCtorInitializer`s are not in it. > > I should use `hasBody()` and pass in an argument. > You are calling `Function-hasBody()` followed by `Function->getDefinition()`, > which is what `FunctionDecl::getBody()` does. The difference is that > `FunctionDecl::getBody()` will also load serialized bodies from the AST (if > the body happens to be in a PCH, for instance). Do I want to load serialized bodies? https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
malcolm.parsons updated this revision to Diff 82239.
malcolm.parsons added a comment.
Move hasBody check into matcher.
The matcher checks the function is a definition.
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 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(Copy)) {}
+ ExpensiveMovableType Field;
};
template
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 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
Index: clang-tidy/utils/DeclRefExprUtils.h
===
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -28,23 +28,34 @@
bool isOnlyUsedAsConst(const VarDecl , const Stmt ,
ASTContext );
-// Returns set of all DeclRefExprs to VarDecl in Stmt.
+/// Returns set of all DeclRefExprs to VarDecl within Stmt.
llvm::SmallPtrSet
allDeclRefExprs(const VarDecl , const Stmt , ASTContext );
-// 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
+allDeclRefExprs(const VarDecl , const Decl , ASTContext );
+
+/// Returns set of all DeclRefExprs to VarDecl within Stmt where VarDecl is
+/// guaranteed to be accessed in a const fashion.
llvm::SmallPtrSet
constReferenceDeclRefExprs(const VarDecl , const Stmt ,
ASTContext );
-// Returns true if DeclRefExpr is the argument of a copy-constructor call expr.
-bool isCopyConstructorArgument(const DeclRefExpr , const Stmt ,
+/// Returns set of all DeclRefExprs to VarDecl within Decl
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
aaron.ballman added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); malcolm.parsons wrote: > malcolm.parsons wrote: > > aaron.ballman wrote: > > > Instead of using `hasBody()` and `getDefinition()`, you should use > > > `FunctionDecl::getBody()` and pass in an argument to receive the > > > function's definition. > > I don't want the body - the `CXXCtorInitializer`s are not in it. > I should use `hasBody()` and pass in an argument. You are calling `Function-hasBody()` followed by `Function->getDefinition()`, which is what `FunctionDecl::getBody()` does. The difference is that `FunctionDecl::getBody()` will also load serialized bodies from the AST (if the body happens to be in a PCH, for instance). https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
malcolm.parsons added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); malcolm.parsons wrote: > aaron.ballman wrote: > > Instead of using `hasBody()` and `getDefinition()`, you should use > > `FunctionDecl::getBody()` and pass in an argument to receive the function's > > definition. > I don't want the body - the `CXXCtorInitializer`s are not in it. I should use `hasBody()` and pass in an argument. https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
malcolm.parsons added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); aaron.ballman wrote: > Instead of using `hasBody()` and `getDefinition()`, you should use > `FunctionDecl::getBody()` and pass in an argument to receive the function's > definition. I don't want the body - the `CXXCtorInitializer`s are not in it. https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
aaron.ballman added inline comments. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:96 - // 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(Function) || -!Function->doesThisDeclarationHaveABody())) -return; + const FunctionDecl = *Function->getDefinition(); Instead of using `hasBody()` and `getDefinition()`, you should use `FunctionDecl::getBody()` and pass in an argument to receive the function's definition. https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
malcolm.parsons updated this revision to Diff 82237.
malcolm.parsons marked 3 inline comments as done.
malcolm.parsons added a comment.
Add some variables.
Improve comments.
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 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(Copy)) {}
+ ExpensiveMovableType Field;
};
template
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 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
Index: clang-tidy/utils/DeclRefExprUtils.h
===
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -28,23 +28,34 @@
bool isOnlyUsedAsConst(const VarDecl , const Stmt ,
ASTContext );
-// Returns set of all DeclRefExprs to VarDecl in Stmt.
+/// Returns set of all DeclRefExprs to VarDecl within Stmt.
llvm::SmallPtrSet
allDeclRefExprs(const VarDecl , const Stmt , ASTContext );
-// 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
+allDeclRefExprs(const VarDecl , const Decl , ASTContext );
+
+/// Returns set of all DeclRefExprs to VarDecl within Stmt where VarDecl is
+/// guaranteed to be accessed in a const fashion.
llvm::SmallPtrSet
constReferenceDeclRefExprs(const VarDecl , const Stmt ,
ASTContext );
-// Returns true if DeclRefExpr is the argument of a copy-constructor call expr.
-bool isCopyConstructorArgument(const DeclRefExpr , const Stmt ,
+/// Returns set of all DeclRefExprs to VarDecl within
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/performance/UnnecessaryValueParamCheck.cpp:114 if (AllDeclRefExprs.size() == 1 && -!hasLoopStmtAncestor(**AllDeclRefExprs.begin(), *Function->getBody(), - *Result.Context) && +!hasLoopStmtAncestor(**AllDeclRefExprs.begin(), + *Function->getDefinition(), *Result.Context) && How about pulling out variables for `**AllDeclRefExprs.begin()` and `*Function->getDefinition()` to remove some boilerplate? Comment at: clang-tidy/utils/DeclRefExprUtils.h:35 +// Returns set of all DeclRefExprs to VarDecl in Decl. +llvm::SmallPtrSet Please convert the comment (and the other ones the patch touches) to doxygen style. Comment at: clang-tidy/utils/DeclRefExprUtils.h:55-56 // Returns true if DeclRefExpr is the argument of a copy-assignment operator // call expr. +bool isCopyAssignmentArgument(const DeclRefExpr , const Decl , The comment is now confusing. It says nothing about `Decl` and doesn't make it clear where the "call expr" (btw, it should either be `CallExpr`, if it references to the corresponding type or "call expression" otherwise) comes from. Same above. https://reviews.llvm.org/D28022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28022: [clang-tidy] Handle constructors in performance-unnecessary-value-param
malcolm.parsons created this revision.
malcolm.parsons added reviewers: aaron.ballman, flx, alexfh.
malcolm.parsons added a subscriber: cfe-commits.
Herald added a subscriber: JDevlieghere.
modernize-pass-by-value doesn't warn about value parameters that
cannot be moved, so performance-unnecessary-value-param should.
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 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(Copy)) {}
+ ExpensiveMovableType Field;
};
template
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 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
Index: clang-tidy/utils/DeclRefExprUtils.h
===
--- clang-tidy/utils/DeclRefExprUtils.h
+++ clang-tidy/utils/DeclRefExprUtils.h
@@ -32,19 +32,29 @@
llvm::SmallPtrSet
allDeclRefExprs(const VarDecl , const Stmt , ASTContext );
+// Returns set of all DeclRefExprs to VarDecl in Decl.
+llvm::SmallPtrSet
+allDeclRefExprs(const VarDecl , const Decl , ASTContext );
+
// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in
// a const fashion.
llvm::SmallPtrSet
constReferenceDeclRefExprs(const VarDecl , const Stmt ,
ASTContext );
+// Returns set of all DeclRefExprs where VarDecl is guaranteed to be accessed in
+// a const fashion.
+llvm::SmallPtrSet
+constReferenceDeclRefExprs(const VarDecl , const Decl ,
+ ASTContext );
+
// Returns true if DeclRefExpr is the argument of a copy-constructor call expr.
-bool isCopyConstructorArgument(const DeclRefExpr , const Stmt ,
+bool
