Add performance check to flag function parameters of expensive to copy types that can be safely converted to const references.

[alexfh]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

42

Please add a comment about when this happens (template parameter packs? C-style variadic functions?).

47

Please add an empty line before the comment.

49

nit: Add three spaces before the first word for better alignment.

59

s/making this a reference/making it a reference/?

63

What if parameter doesn't have a name? Should we print an index ("parameter #n is copied ...").

docs/clang-tidy/checks/performance-unnecessary-value-param.rst

7

Add an example?

17

nit: a ; seems to be more suitable as a trailing punctuation here.

[flx]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

42

I"m not sure when this can happen. Should I remove it and we add it if we find a case?

63

Good point. Added test and index code.

[alexfh]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

25–32

How about return (Name.empty() ? llvm::Twine('#') + (Index + 1) : llvm::Twine('\'') + Name + '\'').str(); or something like this?

42

Can you first try adding tests with template parameter packs and C-style variadic functions?

56

nit: No braces around single-line if bodies.

[fowles]

test/clang-tidy/performance-unnecessary-value-param.cpp

8

you don't actually need to fill in these methods, just declare them

52

no fix for this case?

[flx]

After testing the check against a large corpus I was able to remove the unnecessary param index check. But I discovered that the check crashed on value arguments of deleted assignment operators due to the lack of a function body. I added a test case and added a guard to address this.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

42

Ran over large corpus and the check is not needed. Removed.

56

Code is gone. Was not needed.

test/clang-tidy/performance-unnecessary-value-param.cpp

52

Good point. Added.

[alexfh]

Looks good! Thank you!

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

27

nit: IIUC, there's no need for explicit llvm::Twine around the second '\'' in this line.

[flx]

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

27

Twine only has an implicit constructor for const char* but not for char. I can either leave it as is or switch to "'".

[LegalizeAdulthood]

Can you add something to the docs/ReleaseNotes.rst for this new check, please?

clang-tidy/performance/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyPerformanceModule
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitCastInLoopCheck.cpp
   PerformanceTidyModule.cpp
   UnnecessaryCopyInitialization.cpp
+  UnnecessaryValueParamCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/performance/PerformanceTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 
 #include "FasterStringFindCheck.h"
 #include "ForRangeCopyCheck.h"
 #include "ImplicitCastInLoopCheck.h"
 #include "UnnecessaryCopyInitialization.h"
+#include "UnnecessaryValueParamCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace performance {
 
 class PerformanceModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<FasterStringFindCheck>(
         "performance-faster-string-find");
     CheckFactories.registerCheck<ForRangeCopyCheck>(
         "performance-for-range-copy");
     CheckFactories.registerCheck<ImplicitCastInLoopCheck>(
         "performance-implicit-cast-in-loop");
     CheckFactories.registerCheck<UnnecessaryCopyInitialization>(
         "performance-unnecessary-copy-initialization");
+    CheckFactories.registerCheck<UnnecessaryValueParamCheck>(
+        "performance-unnecessary-value-param");
   }
 };
 
 // Register the PerformanceModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<PerformanceModule>
     X("performance-module", "Adds performance checks.");
 
 } // namespace performance
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the PerformanceModule.
 volatile int PerformanceModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/performance/UnnecessaryValueParamCheck.h

+//===--- UnnecessaryValueParamCheck.h - clang-tidy---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+/// \brief A check that flags value parameters of expensive to copy types that
+/// can safely be converted to const references.
+///
+/// For the user-facing documentation see:
+/// 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) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_VALUE_PARAM_H

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

+//===--- UnnecessaryValueParamCheck.cpp - clang-tidy-----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnnecessaryValueParamCheck.h"
+
+#include "../utils/DeclRefExprUtils.h"
+#include "../utils/FixItHintUtils.h"
+#include "../utils/Matchers.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+namespace {
+
+std::string paramNameOrIndex(StringRef Name, size_t Index) {
+  std::string Output;
+  llvm::raw_string_ostream Stream(Output);
+  if (!Name.empty()) {
+    Stream << "'" << Name << "'";
+  } else {
+    Stream << "#" << ++Index;
+  }
+  return Stream.str();
+}
+
+} // namespace
+
+void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ExpensiveValueParamDecl =
+      parmVarDecl(hasType(hasCanonicalType(allOf(matchers::isExpensiveToCopy(),
+                                                 unless(referenceType())))),
+                  decl().bind("param"));
+  Finder->addMatcher(
+      functionDecl(isDefinition(), unless(cxxMethodDecl(isOverride())),
+                   unless(isInstantiated()),
+                   has(typeLoc(forEach(ExpensiveValueParamDecl))),
+                   decl().bind("functionDecl")),
+      this);
+}
+
+void UnnecessaryValueParamCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
+  const auto *Function = Result.Nodes.getNodeAs<FunctionDecl>("functionDecl");
+  const size_t Index = std::find(Function->parameters().begin(),
+                                 Function->parameters().end(), Param) -
+                       Function->parameters().begin();
+  if (Index >= Function->getNumParams()) {
+    return;
+  }
+  bool IsConstQualified =
+      Param->getType().getCanonicalType().isConstQualified();
+
+  // 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) ||
+                            !decl_ref_expr_utils::isOnlyUsedAsConst(
+                                *Param, *Function->getBody(), *Result.Context)))
+    return;
+  auto Diag =
+      diag(Param->getLocation(),
+           IsConstQualified ? "the const qualified parameter %0 is "
+                              "copied for each invocation; consider "
+                              "making it a reference"
+                            : "the parameter %0 is copied for each "
+                              "invocation but only used as a const reference; "
+                              "consider making it a const reference")
+      << paramNameOrIndex(Param->getName(), Index);
+  // Do not propose fixes in macros since we cannot place them correctly.
+  if (Param->getLocStart().isMacroID())
+    return;
+  for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
+       FunctionDecl = FunctionDecl->getPreviousDecl()) {
+    const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
+    Diag << utils::create_fix_it::changeVarDeclToReference(CurrentParam,
+                                                           *Result.Context);
+    if (!IsConstQualified)
+      Diag << utils::create_fix_it::changeVarDeclToConst(CurrentParam);
+  }
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    modernize-shrink-to-fit
    modernize-use-auto
    modernize-use-default
    modernize-use-nullptr
    modernize-use-override
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-cast-in-loop
+   performance-unnecessary-value-param
    readability-braces-around-statements
    readability-container-size-empty
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-cast
    readability-inconsistent-declaration-parameter-name
    readability-named-parameter
    readability-redundant-control-flow
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-simplify-boolean-expr
    readability-uniqueptr-delete-release

docs/clang-tidy/checks/performance-unnecessary-value-param.rst

+.. title:: clang-tidy - performance-unnecessary-value-param
+
+performance-unnecessary-value-param
+===================================
+
+Flags value parameter declarations of expensive to copy types that are copied
+for each invocation but it would suffice to pass them by const reference.
+
+The check is only applied to parameters of types that are expensive to copy
+which means they are not trivially copyable or have a non-trivial copy
+constructor or destructor.
+
+To ensure that it is safe to replace the value paramater with a const reference
+the following heuristic is employed:
+
+1. the parameter is const qualified;
+2. the parameter is not const, but only const methods or operators are invoked
+   on it, or it is used as const reference or value argument in constructors or
+   function calls.
+
+Example:
+
+.. code-block:: c++
+
+  void f(const string Value) {
+    // The warning will suggest making Value a reference.
+  }
+
+  void g(ExpensiveToCopy Value) {
+    // The warning will suggest making Value a const reference.
+    Value.ConstMethd();
+    ExpensiveToCopy Copy(Value);
+  }

test/clang-tidy/performance-unnecessary-value-param.cpp

+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+
+struct ExpensiveToCopyType {
+  const ExpensiveToCopyType & constReference() const {
+    return *this;
+  }
+  void nonConstMethod() {}
+  virtual ~ExpensiveToCopyType() {}
+};
+
+void mutate(ExpensiveToCopyType &);
+void mutate(ExpensiveToCopyType *);
+void useAsConstReference(const ExpensiveToCopyType &);
+void useByValue(ExpensiveToCopyType);
+
+// This class simulates std::pair<>. It is trivially copy constructible
+// and trivially destructible, but not trivially copy assignable.
+class SomewhatTrivial {
+ public:
+  SomewhatTrivial();
+  SomewhatTrivial(const SomewhatTrivial&) = default;
+  ~SomewhatTrivial() = default;
+  SomewhatTrivial& operator=(const SomewhatTrivial&);
+};
+
+void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
+// CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj);
+void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) {
+  // CHECK-MESSAGES: [[@LINE-1]]:60: warning: the const qualified parameter 'Obj' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
+  // CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj) {
+}
+
+void positiveExpensiveValue(ExpensiveToCopyType Obj);
+// CHECK-FIXES: void positiveExpensiveValue(const ExpensiveToCopyType& Obj);
+void positiveExpensiveValue(ExpensiveToCopyType Obj) {
+  // CHECK-MESSAGES: [[@LINE-1]]:49: warning: the parameter 'Obj' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
+  // CHECK-FIXES: void positiveExpensiveValue(const ExpensiveToCopyType& Obj) {
+  Obj.constReference();
+  useAsConstReference(Obj);
+  auto Copy = Obj;
+  useByValue(Obj);
+}
+
+void positiveWithComment(const ExpensiveToCopyType /* important */ S);
+// CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S);
+void positiveWithComment(const ExpensiveToCopyType /* important */ S) {
+  // CHECK-MESSAGES: [[@LINE-1]]:68: warning: the const qualified
+  // CHECK-FIXES: void positiveWithComment(const ExpensiveToCopyType& /* important */ S) {
+}
+
+void positiveUnnamedParam(const ExpensiveToCopyType) {
+  // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter #1
+}
+
+void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy);
+// CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy);
+void positiveAndNegative(const ExpensiveToCopyType ConstCopy, const ExpensiveToCopyType& ConstRef, ExpensiveToCopyType Copy) {
+  // CHECK-MESSAGES: [[@LINE-1]]:52: warning: the const qualified parameter 'ConstCopy'
+  // CHECK-MESSAGES: [[@LINE-2]]:120: warning: the parameter 'Copy'
+  // CHECK-FIXES: void positiveAndNegative(const ExpensiveToCopyType& ConstCopy, const ExpensiveToCopyType& ConstRef, const ExpensiveToCopyType& Copy) {
+}
+
+struct PositiveConstValueConstructor {
+  PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
+};
+
+template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
+  // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
+  // CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+}
+
+void instantiated() {
+  templateWithNonTemplatizedParameter(ExpensiveToCopyType(), ExpensiveToCopyType());
+  templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5);
+}
+
+template <typename T> void negativeTemplateType(const T V) {
+}
+
+void negativeArray(const ExpensiveToCopyType[]) {
+}
+
+void negativePointer(ExpensiveToCopyType* Obj) {
+}
+
+void negativeConstPointer(const ExpensiveToCopyType* Obj) {
+}
+
+void negativeConstReference(const ExpensiveToCopyType& Obj) {
+}
+
+void negativeReference(ExpensiveToCopyType& Obj) {
+}
+
+void negativeUniversalReference(ExpensiveToCopyType&& Obj) {
+}
+
+void negativeSomewhatTrivialConstValue(const SomewhatTrivial Somewhat) {
+}
+
+void negativeSomewhatTrivialValue(SomewhatTrivial Somewhat) {
+}
+
+void negativeConstBuiltIn(const int I) {
+}
+
+void negativeValueBuiltIn(int I) {
+}
+
+void negativeValueIsMutatedByReference(ExpensiveToCopyType Obj) {
+  mutate(Obj);
+}
+
+void negativeValueIsMutatatedByPointer(ExpensiveToCopyType Obj) {
+  mutate(&Obj);
+}
+
+void negativeValueIsReassigned(ExpensiveToCopyType Obj) {
+  Obj = ExpensiveToCopyType();
+}
+
+void negativeValueNonConstMethodIsCalled(ExpensiveToCopyType Obj) {
+  Obj.nonConstMethod();
+}
+
+struct NegativeValueCopyConstructor {
+  NegativeValueCopyConstructor(ExpensiveToCopyType Copy) {}
+};
+
+template <typename T>
+struct Container {
+  typedef const T & const_reference;
+};
+
+void NegativeTypedefParam(const Container<ExpensiveToCopyType>::const_reference Param) {
+}
+
+#define UNNECESSARY_VALUE_PARAM_IN_MACRO_BODY()         \
+  void inMacro(const ExpensiveToCopyType T) {           \
+  }                                                     \
+// Ensure fix is not applied.
+// CHECK-FIXES: void inMacro(const ExpensiveToCopyType T) {
+
+UNNECESSARY_VALUE_PARAM_IN_MACRO_BODY()
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: the const qualified parameter 'T'
+
+#define UNNECESSARY_VALUE_PARAM_IN_MACRO_ARGUMENT(ARGUMENT)     \
+  ARGUMENT
+
+UNNECESSARY_VALUE_PARAM_IN_MACRO_ARGUMENT(void inMacroArgument(const ExpensiveToCopyType InMacroArg) {})
+// CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'InMacroArg'
+// CHECK-FIXES: void inMacroArgument(const ExpensiveToCopyType InMacroArg) {}
+
+struct VirtualMethod {
+  virtual ~VirtualMethod() {}
+  virtual void handle(ExpensiveToCopyType T) const = 0;
+};
+
+struct NegativeOverriddenMethod : public VirtualMethod {
+  void handle(ExpensiveToCopyType Overridden) const {
+    // CHECK-FIXES: handle(ExpensiveToCopyType Overridden) const {
+  }
+};