[clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved.

Summary:

Make check more useful in the following two cases:

The parameter is passed by non-const value, has a non-deleted move constructor and is only referenced once in the function as argument to the type's copy constructor.
The parameter is passed by non-const value, has a non-deleted move assignment operator and is only referenced once in the function as argument of the the type's copy assignment operator.
In this case suggest a fix to move the parameter which avoids the unnecessary copy and is closest to what the user might have intended.

Reviewers: alexfh, sbenza

Subscribers: cfe-commits, Prazek

Differential Revision: http://reviews.llvm.org/D20277

git-svn-id: https://llvm.org/svn/llvm-project/clang-tools-extra/trunk@274380 91177308-0d34-0410-b5e6-96231b3b80d8

2 files changed, 94 insertions, 6 deletions

diff --git a/clang-tidy/performance/UnnecessaryValueParamCheck.cpp b/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
index 276ffd82..d3e4c7fb 100644
--- a/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
+++ b/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
@@ -12,6 +12,10 @@
 #include "../utils/DeclRefExprUtils.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/Matchers.h"
+#include "../utils/TypeTraits.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
 
 using namespace clang::ast_matchers;
 
@@ -27,8 +31,22 @@ std::string paramNameOrIndex(StringRef Name, size_t Index) {
       .str();
 }
 
+template <typename S>
+bool isSubset(const S &SubsetCandidate, const S &SupersetCandidate) {
+  for (const auto &E : SubsetCandidate)
+    if (SupersetCandidate.count(E) == 0)
+      return false;
+  return true;
+}
+
 } // namespace
 
+UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+          Options.get("IncludeStyle", "llvm"))) {}
+
 void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
   const auto ExpensiveValueParamDecl =
       parmVarDecl(hasType(hasCanonicalType(allOf(matchers::isExpensiveToCopy(),
@@ -58,12 +76,39 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   // Do not trigger on non-const value parameters when:
   // 1. they are in a constructor definition since they can likely trigger
   //    misc-move-constructor-init which will suggest to move the argument.
-  // 2. they are not only used as const.
   if (!IsConstQualified && (llvm::isa<CXXConstructorDecl>(Function) ||
-                            !Function->doesThisDeclarationHaveABody() ||
-                            !utils::decl_ref_expr::isOnlyUsedAsConst(
-                                *Param, *Function->getBody(), *Result.Context)))
+                            !Function->doesThisDeclarationHaveABody()))
     return;
+
+  auto AllDeclRefExprs = utils::decl_ref_expr::allDeclRefExprs(
+      *Param, *Function->getBody(), *Result.Context);
+  auto ConstDeclRefExprs = utils::decl_ref_expr::constReferenceDeclRefExprs(
+      *Param, *Function->getBody(), *Result.Context);
+  // 2. 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) {
+    auto CanonicalType = Param->getType().getCanonicalType();
+    if (AllDeclRefExprs.size() == 1 &&
+        ((utils::type_traits::hasNonTrivialMoveConstructor(CanonicalType) &&
+          utils::decl_ref_expr::isCopyConstructorArgument(
+              **AllDeclRefExprs.begin(), *Function->getBody(),
+              *Result.Context)) ||
+         (utils::type_traits::hasNonTrivialMoveAssignment(CanonicalType) &&
+          utils::decl_ref_expr::isCopyAssignmentArgument(
+              **AllDeclRefExprs.begin(), *Function->getBody(),
+              *Result.Context)))) {
+      handleMoveFix(*Param, **AllDeclRefExprs.begin(), *Result.Context);
+      return;
+    }
+  }
+
   auto Diag =
       diag(Param->getLocation(),
            IsConstQualified ? "the const qualified parameter %0 is "
@@ -86,6 +131,40 @@ void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
   }
 }
 
+void UnnecessaryValueParamCheck::registerPPCallbacks(
+    CompilerInstance &Compiler) {
+  Inserter.reset(new utils::IncludeInserter(
+      Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle));
+  Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks());
+}
+
+void UnnecessaryValueParamCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle",
+                utils::IncludeSorter::toString(IncludeStyle));
+}
+
+void UnnecessaryValueParamCheck::handleMoveFix(const ParmVarDecl &Var,
+                                               const DeclRefExpr &CopyArgument,
+                                               const ASTContext &Context) {
+  auto Diag = diag(CopyArgument.getLocStart(),
+                   "parameter %0 is passed by value and only copied once; "
+                   "consider moving it to avoid unnecessary copies")
+              << &Var;
+  // Do not propose fixes in macros since we cannot place them correctly.
+  if (CopyArgument.getLocStart().isMacroID())
+    return;
+  const auto &SM = Context.getSourceManager();
+  auto EndLoc = Lexer::getLocForEndOfToken(CopyArgument.getLocation(), 0, SM, 
+					   Context.getLangOpts());
+  Diag << FixItHint::CreateInsertion(CopyArgument.getLocStart(), "std::move(")
+       << FixItHint::CreateInsertion(EndLoc, ")");
+  if (auto IncludeFixit = Inserter->CreateIncludeInsertion(
+          SM.getFileID(CopyArgument.getLocStart()), "utility",
+          /*IsAngled=*/true))
+    Diag << *IncludeFixit;
+}
+
 } // namespace performance
 } // namespace tidy
 } // namespace clang
diff --git a/clang-tidy/performance/UnnecessaryValueParamCheck.h b/clang-tidy/performance/UnnecessaryValueParamCheck.h
index 05e60e20..cbf0a3bb 100644
--- a/clang-tidy/performance/UnnecessaryValueParamCheck.h
+++ b/clang-tidy/performance/UnnecessaryValueParamCheck.h
@@ -11,6 +11,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H
 
 #include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
 
 namespace clang {
 namespace tidy {
@@ -23,10 +24,18 @@ namespace performance {
 /// http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-value-param.html
 class UnnecessaryValueParamCheck : public ClangTidyCheck {
 public:
-  UnnecessaryValueParamCheck(StringRef Name, ClangTidyContext *Context)
-      : ClangTidyCheck(Name, Context) {}
+  UnnecessaryValueParamCheck(StringRef Name, ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void registerPPCallbacks(CompilerInstance &Compiler) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  void handleMoveFix(const ParmVarDecl &Var, const DeclRefExpr &CopyArgument,
+                     const ASTContext &Context);
+
+  std::unique_ptr<utils::IncludeInserter> Inserter;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
 };
 
 } // namespace performance