[clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[baloghadamsoftware]

Summary

The three checks mentioned in the Title are two noisy if the code uses custom (e.g. smart) pointers or references. LLVM/Clang is such a code, it has lots of such types, e.g. StringRef, SymbolRef or ProgramStateRef, all of them based llvm::IntrusiveRefCntPtr. Every time such a type is passed as parameter, used for initialization or in a range-based for loop a false positive warning is issued together with an (unnecessary) fix.

This patch excludes pointer types (e.g. smart pointers) by their name: if the name of the type ends on Pointer, pointer, Ptr, ptr, Reference, reference, Ref or ref the type is considered a pointer/reference type.

[baloghadamsoftware]

This is an alternative approach to D52315.

[lebedev.ri]

Better, but these blacklists need to be config options, not random hardcoded globs.
See e.g. google-runtime-references.WhiteListTypes.

[baloghadamsoftware]

Config option is a good idea but it must not be empty by default. The checker must ignore all pointer and references by default based on their names.

[danilaml]

Would that also skip checks for something like shared_ptr?

[lebedev.ri]

In D52360#1246440, @baloghadamsoftware wrote:

Config option is a good idea but it must not be empty by default. The checker must ignore all pointer and references by default based on their names.

I disagree, it must not have false-negatives by default.
It may have false-positives, that are controllable via the options.

[baloghadamsoftware]

In D52360#1246443, @danilaml wrote:

Would that also skip checks for something like shared_ptr?

Yes, everything ending on pointer, ptr, reference or ref, first letter case insensitive.

[baloghadamsoftware]

In D52360#1246452, @lebedev.ri wrote:

I disagree, it must not have false-negatives by default.

What kind of false-negatives could be caused by smart pointers or references? Sane people do not name a type pointer or reference if they are not. Such types must never be passed by reference. Few people will use the check if they have to set tons of options for the basic expected behavior. For example in CodeChecker this check is disabled by default and is only enabled in the Sensitive profile.

[lebedev.ri]

In D52360#1246474, @baloghadamsoftware wrote:

In D52360#1246452, @lebedev.ri wrote:

I disagree, it must not have false-negatives by default.

What kind of false-negatives could be caused by smart pointers or references? Sane people do not name a type pointer or reference if they are not. Such types must never be passed by reference. Few people will use the check if they have to set tons of options for the basic expected behavior. For example in CodeChecker this check is disabled by default and is only enabled in the Sensitive profile.

Ok, so let's entertain the current patch.
Now, some time passes, and someone either discovers that the check does not warn on some code that one would have thought it should warn.
Shitty code happens. Not everyone is sane in their naming choices. More importantly, there isn't a common coding style naming guidelines everyone has to follow.
Someone somewhere will surely hit this. When he does, how does one get the check to warn on that?
Or other side of the coin, how does one get the check not to warn on some custom type that does not match those hardcoded regexes?

I don't disagree that this has false-positives. But this is really sensitive to the type names. It is not a good idea to hardcode this.
Please follow pre-existing practice of configurable type white/blacklists.

$ clang-tidy -checks=\* -dump-config | grep -i "types$" -A1
  - key:             cert-msc32-c.DisallowedSeedTypes
    value:           'time_t,std::time_t'
  - key:             cert-msc51-cpp.DisallowedSeedTypes
    value:           'time_t,std::time_t'
--
  - key:             google-runtime-references.WhiteListTypes
    value:           ''
--
  - key:             hicpp-use-emplace.TupleTypes
    value:           '::std::pair;::std::tuple'
--
  - key:             modernize-use-emplace.TupleTypes
    value:           '::std::pair;::std::tuple'
--
  - key:             readability-simplify-subscript-expr.Types
    value:           '::std::basic_string;::std::basic_string_view;::std::vector;::std::array'

[flx]

In D52360#1246472, @baloghadamsoftware wrote:

In D52360#1246443, @danilaml wrote:

Would that also skip checks for something like shared_ptr?

Yes, everything ending on pointer, ptr, reference or ref, first letter case insensitive.

std::shared_ptr should not be blacklisted. It is not free to copy it: It incurs two atomic operations and two branches.

Users should blacklist it if they know they don't care about this or not use the check.

While it looks weird for the check to suggest to pass std::shared_ptr by reference it is correct. A better change would be to just pass the raw pointer in this case:

std::shared_ptr<Ptr> p;
PassByRawPointer(p.get());

But this would require function signature change that breaks callers outside of the translation unit.

clang-tidy/performance/ForRangeCopyCheck.cpp

void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies);
 }
 
 void ForRangeCopyCheck::registerMatchers(MatchFinder *Finder) {
   // Match loop variables that are not references or pointers or are already
   // initialized through MaterializeTemporaryExpr which indicates a type
   // conversion.
   auto LoopVar = varDecl(
-      hasType(hasCanonicalType(unless(anyOf(referenceType(), pointerType())))),
+      hasType(hasCanonicalType(
+          unless(anyOf(referenceType(), pointerType(),
+                       hasDeclaration(namedDecl(matchesName(
+                           "[Pp]ointer$|[Pp]tr$|[Rr]ef(erence)?$"))))))),
       unless(hasInitializer(expr(hasDescendant(materializeTemporaryExpr())))));
   Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar")))
                          .bind("forRange"),
                      this);
 }
 
 void ForRangeCopyCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Var = Result.Nodes.getNodeAs<VarDecl>("loopVar");

clang-tidy/performance/UnnecessaryCopyInitialization.cpp

auto ConstRefReturningFunctionCall =
       callExpr(callee(functionDecl(returns(ConstReference))),
                unless(callee(cxxMethodDecl())));
 
   auto localVarCopiedFrom = [](const internal::Matcher<Expr> &CopyCtorArg) {
     return compoundStmt(
                forEachDescendant(
                    declStmt(
                        has(varDecl(hasLocalStorage(),
-                                   hasType(matchers::isExpensiveToCopy()),
+                                   hasType(hasCanonicalType(allOf(
+                                       matchers::isExpensiveToCopy(),
+                                       unless(hasDeclaration(namedDecl(
+                                           matchesName("[Pp]ointer$|[Pp]tr$|["
+                                                       "Rr]ef(erence)?$"))))))),
                                    unless(isImplicit()),
                                    hasInitializer(
                                        cxxConstructExpr(
                                            hasDeclaration(cxxConstructorDecl(
                                                isCopyConstructor())),
                                            hasArgument(0, CopyCtorArg))
                                            .bind("ctorCall")))
                                .bind("newVarDecl")))

clang-tidy/performance/UnnecessaryValueParamCheck.cpp

: ClangTidyCheck(Name, Context),
       IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
           Options.getLocalOrGlobal("IncludeStyle", "llvm"))) {}
 
 void UnnecessaryValueParamCheck::registerMatchers(MatchFinder *Finder) {
   // This check is specific to C++ and doesn't apply to languages like
   // Objective-C.
   if (!getLangOpts().CPlusPlus)
     return;
-  const auto ExpensiveValueParamDecl =
-      parmVarDecl(hasType(hasCanonicalType(allOf(
-                      unless(referenceType()), matchers::isExpensiveToCopy()))),
-                  decl().bind("param"));
+  const auto ExpensiveValueParamDecl = parmVarDecl(
+      hasType(hasCanonicalType(
+          allOf(unless(anyOf(referenceType(),
+                             hasDeclaration(namedDecl(matchesName(
+                                 "[Pp]ointer$|[Pp]tr$|[Rr]ef(erence)?$"))))),
+                matchers::isExpensiveToCopy()))),
   Finder->addMatcher(
       functionDecl(hasBody(stmt()), isDefinition(), unless(isImplicit()),
                    unless(cxxMethodDecl(anyOf(isOverride(), isFinal()))),
                    has(typeLoc(forEach(ExpensiveValueParamDecl))),
                    unless(isInstantiated()), decl().bind("functionDecl")),
       this);
 }
 

docs/clang-tidy/checks/performance-for-range-copy.rst

 .. title:: clang-tidy - performance-for-range-copy
 
 performance-for-range-copy
 ==========================
 
 Finds C++11 for ranges where the loop variable is copied in each iteration but
 it would suffice to obtain it by const reference.
 
 The check is only applied to loop variables of types that are expensive to copy
 which means they are not trivially copyable or have a non-trivial copy
 constructor or destructor.
 
+To avoid too many false-positives the check ignores types considered as pointer
+or reference types (e.g. smart pointers) based on their name. The name of these
+types must end to either `Pointer`, `pointer`, `Ptr`, `ptr`, `Reference`,
+`reference`, `Ref`, `ref`.
+
 To ensure that it is safe to replace the copy with a const reference the
 following heuristic is employed:
 
 1. The loop variable is const qualified.
 2. The loop variable 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.
 
 Options
 -------
 
 .. option:: WarnOnAllAutoCopies
 
    When non-zero, warns on any use of `auto` as the type of the range-based for
    loop variable. Default is `0`.

docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst

 .. title:: clang-tidy - performance-unnecessary-copy-initialization
 
 performance-unnecessary-copy-initialization
 ===========================================
 
 Finds local variable declarations that are initialized using the copy
 constructor of a non-trivially-copyable type but it would suffice to obtain a
 const reference.
 
+To avoid too many false-positives the check ignores types considered as pointer
+or reference types (e.g. smart pointers) based on their name. The name of these
+types must end to either `Pointer`, `pointer`, `Ptr`, `ptr`, `Reference`,
+`reference`, `Ref`, `ref`.
+
 The check is only applied if it is safe to replace the copy by a const
 reference. This is the case when the variable is const qualified or when it is
 only used as a const, i.e. 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:
 

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 avoid too many false-positives the check ignores types considered as pointer
+or reference types (e.g. smart pointers) based on their name. The name of these
+types must end to either `Pointer`, `pointer`, `Ptr`, `ptr`, `Reference`,
+`reference`, `Ref`, `ref`.
+
 To ensure that it is safe to replace the value parameter 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.
 

test/clang-tidy/performance-for-range-copy.cpp

 };
 
 struct Convertible {
   operator S() const {
     return S();
   }
 };
 
+struct SmartPointer {
+  ~SmartPointer();
+};
+
+struct smart_pointer {
+  ~smart_pointer();
+};
+
+struct SmartPtr {
+  ~SmartPtr();
+};
+
+struct smart_ptr {
+  ~smart_ptr();
+};
+
+struct SmartReference {
+  ~SmartReference();
+};
+
+struct smart_reference {
+  ~smart_reference();
+};
+
+struct SmartRef {
+  ~SmartRef();
+};
+
+struct smart_ref {
+  ~smart_ref();
+};
+
 void negativeConstReference() {
   for (const S &S1 : View<Iterator<S>>()) {
   }
 }
 
 void negativeUserDefinedConversion() {
   Convertible C[0];
   for (const S &S1 : C) {
for (auto ConstOperatorInvokee : View<Iterator<Mutable>>()) {
     bool result = ConstOperatorInvokee != Mutable();
   }
 }
 
 void IgnoreLoopVariableNotUsedInLoopBody() {
   for (auto _ : View<Iterator<S>>()) {
   }
 }
+
+void negativeSmartPointer() {
+  for (auto P : View<Iterator<SmartPointer>>()) {
+    auto P2 = P;
+  }
+}
+
+void negative_smart_pointer() {
+  for (auto p : View<Iterator<smart_pointer>>()) {
+    auto p2 = p;
+  }
+}
+
+void negativeSmartPtr() {
+  for (auto P : View<Iterator<SmartPtr>>()) {
+    auto P2 = P;
+  }
+}
+
+void negative_smart_ptr() {
+  for (auto p : View<Iterator<smart_ptr>>()) {
+    auto p2 = p;
+  }
+}
+
+void negativeSmartReference() {
+  for (auto R : View<Iterator<SmartReference>>()) {
+    auto R2 = R;
+  }
+}
+
+void negative_smart_reference() {
+  for (auto r : View<Iterator<smart_reference>>()) {
+    auto r2 = r;
+  }
+}
+
+void negativeSmartRef() {
+  for (auto R : View<Iterator<SmartRef>>()) {
+    auto R2 = R;
+  }
+}
+
+void negative_smart_ref() {
+  for (auto r : View<Iterator<smart_ref>>()) {
+    auto r2 = r;
+  }
+}

test/clang-tidy/performance-unnecessary-copy-initialization.cpp

 struct WeirdCopyCtorType {
   WeirdCopyCtorType();
   WeirdCopyCtorType(const WeirdCopyCtorType &w, bool oh_yes = true);
 
   void nonConstMethod();
   bool constMethod() const;
 };
 
+struct SmartPointer {
+  ~SmartPointer();
+};
+
+struct smart_pointer {
+  ~smart_pointer();
+};
+
+struct SmartPtr {
+  ~SmartPtr();
+};
+
+struct smart_ptr {
+  ~smart_ptr();
+};
+
+struct SmartReference {
+  ~SmartReference();
+};
+
+struct smart_reference {
+  ~smart_reference();
+};
+
+struct SmartRef {
+  ~SmartRef();
+};
+
+struct smart_ref {
+  ~smart_ref();
+};
+
 ExpensiveToCopyType global_expensive_to_copy_type;
 
 const ExpensiveToCopyType &ExpensiveTypeReference();
 const TrivialToCopyType &TrivialTypeReference();
 
+const SmartPointer &getSmartPointer();
+const smart_pointer &get_smart_pointer();
+const SmartPtr &getSmartPtr();
+const smart_ptr &get_smart_ptr();
+const SmartReference &getSmartReference();
+const smart_reference &get_smart_reference();
+const SmartRef &getSmartRef();
+const smart_ref &get_smart_ref();
+
 void mutate(ExpensiveToCopyType &);
 void mutate(ExpensiveToCopyType *);
 void useAsConstPointer(const ExpensiveToCopyType *);
 void useAsConstReference(const ExpensiveToCopyType &);
 void useByValue(ExpensiveToCopyType);
 
 void PositiveFunctionCall() {
   const auto AutoAssigned = ExpensiveTypeReference();
public:
   const Iterator &begin() const;
   const Iterator &end() const;
 };
 
 void implicitVarFalsePositive() {
   for (const Element &E : Container()) {
   }
 }
+
+void negativeSmartPointer() {
+  const auto P = getSmartPointer();
+}
+
+void negative_smart_pointer() {
+  const auto p = get_smart_pointer();
+}
+
+void negativeSmartPtr() {
+  const auto P = getSmartPtr();
+}
+
+void negative_smart_ptr() {
+  const auto p = get_smart_ptr();
+}
+
+void negativeSmartReference() {
+  const auto R = getSmartReference();
+}
+
+void negative_smart_reference() {
+  const auto r = get_smart_reference();
+}
+
+void negativeSmartRef() {
+  const auto R = getSmartRef();
+}
+
+void negative_smart_ref() {
+  const auto r = get_smart_ref();
+}

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

struct ExpensiveMovableType {
   ExpensiveMovableType();
   ExpensiveMovableType(ExpensiveMovableType &&);
   ExpensiveMovableType(const ExpensiveMovableType &) = default;
   ExpensiveMovableType &operator=(const ExpensiveMovableType &) = default;
   ExpensiveMovableType &operator=(ExpensiveMovableType &&);
   ~ExpensiveMovableType();
 };
 
+struct SmartPointer {
+  ~SmartPointer();
+};
+
+struct smart_pointer {
+  ~smart_pointer();
+};
+
+struct SmartPtr {
+  ~SmartPtr();
+};
+
+struct smart_ptr {
+  ~smart_ptr();
+};
+
+struct SmartReference {
+  ~SmartReference();
+};
+
+struct smart_reference {
+  ~smart_reference();
+};
+
+struct SmartRef {
+  ~SmartRef();
+};
+
+struct smart_ref {
+  ~smart_ref();
+};
+
 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);

 }
 
 template<>
 void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
   // CHECK-MESSAGES: [[@LINE-1]]:64: warning: the parameter 'E' is copied
   // CHECK-FIXES: void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+void negativeSmartPointer(SmartPointer P) {
+}
+
+void negative_smart_pointer(smart_pointer p) {
+}
+
+void negativeSmartPtr(SmartPtr P) {
+}
+
+void negative_smart_ptr(smart_ptr p) {
+}
+
+void negativeSmartReference(SmartReference R) {
+}
+
+void negative_smart_reference(smart_reference r) {
+}
+
+void negativeSmartRef(SmartRef R) {
+}
+
+void negative_smart_ref(smart_ref r) {
+}