[PATCH] D50102: Use ExprMutationAnalyzer in performance-unnecessary-value-param

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

It is great when the test cases are included :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50102



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50102: Use ExprMutationAnalyzer in performance-unnecessary-value-param

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang created this revision.
Herald added a reviewer: george.karpenkov.
Herald added subscribers: cfe-commits, a.sidorin.

This yields better recall as ExprMutationAnalyzer is more accurate.

One common pattern this check is now able to catch is:

  void foo(std::vector v) {
for (const auto& elm : v) {
  // ...
}
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50102

Files:
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp


Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -10,6 +10,7 @@
 #include "UnnecessaryValueParamCheck.h"
 
 #include "../utils/DeclRefExprUtils.h"
+#include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/Matchers.h"
 #include "../utils/TypeTraits.h"
@@ -98,43 +99,52 @@
 void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult ) 
{
   const auto *Param = Result.Nodes.getNodeAs("param");
   const auto *Function = Result.Nodes.getNodeAs("functionDecl");
-  const size_t Index = std::find(Function->parameters().begin(),
- Function->parameters().end(), Param) -
-   Function->parameters().begin();
-  bool IsConstQualified =
-  Param->getType().getCanonicalType().isConstQualified();
-
-  auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
-  *Param, *Function, *Result.Context);
-  auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
-  *Param, *Function, *Result.Context);
 
   // Do not trigger on non-const value parameters when they are not only used 
as
   // const.
-  if (!isSubset(AllDeclRefExprs, ConstDeclRefExprs))
+  if (utils::ExprMutationAnalyzer(Function->getBody(), Result.Context)
+  .isMutated(Param))
 return;
+  if (const auto *Ctor = dyn_cast(Function)) {
+for (const auto *Init : Ctor->inits()) {
+  if (utils::ExprMutationAnalyzer(Init->getInit(), Result.Context)
+  .isMutated(Param))
+return;
+}
+  }
+
+  const bool IsConstQualified =
+  Param->getType().getCanonicalType().isConstQualified();
 
   // 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 && AllDeclRefExprs.size() == 1) {
-auto CanonicalType = Param->getType().getCanonicalType();
-const auto   = **AllDeclRefExprs.begin();
-
-if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
-((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
-  utils::decl_ref_expr::isCopyConstructorArgument(
-  DeclRefExpr, *Function, *Result.Context)) ||
- (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
-  utils::decl_ref_expr::isCopyAssignmentArgument(
-  DeclRefExpr, *Function, *Result.Context {
-  handleMoveFix(*Param, DeclRefExpr, *Result.Context);
-  return;
+  if (!IsConstQualified) {
+auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
+*Param, *Function, *Result.Context);
+if (AllDeclRefExprs.size() == 1) {
+  auto CanonicalType = Param->getType().getCanonicalType();
+  const auto  = **AllDeclRefExprs.begin();
+
+  if (!hasLoopStmtAncestor(DeclRefExpr, *Function, *Result.Context) &&
+  ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
+utils::decl_ref_expr::isCopyConstructorArgument(
+DeclRefExpr, *Function, *Result.Context)) ||
+   (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
+utils::decl_ref_expr::isCopyAssignmentArgument(
+DeclRefExpr, *Function, *Result.Context {
+handleMoveFix(*Param, DeclRefExpr, *Result.Context);
+return;
+  }
 }
   }
 
+  const size_t Index = std::find(Function->parameters().begin(),
+ Function->parameters().end(), Param) -
+   Function->parameters().begin();
+
   auto Diag =
   diag(Param->getLocation(),
IsConstQualified ? "the const qualified parameter %0 is "


Index: clang-tidy/performance/UnnecessaryValueParamCheck.cpp
===
--- clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -10,6 +10,7 @@
 #include "UnnecessaryValueParamCheck.h"
 
 #include "../utils/DeclRefExprUtils.h"
+#include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include