[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 intrusive smart (reference counting) pointers. 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 changes the term "expensive to copy" to also regard the size of the type so it disables warnings and fixes if the type is not greater than a pointer. This removes false positives for intrusive smart pointers. False positives for non-intrusive smart pointers are not addressed in this patch.

[JonasToth]

clang-tidy/utils/TypeTraits.cpp

47

Please make that comment a sentence, and maybe don't use not twice.

What do you think about making this parameter configurable and/or set it in relation to the size of a cache-line.
This is of course target dependent, but given the CPU effects more accurate.

[lebedev.ri]

This looks questionable to me.
I don't disagree with the reasons stated about llvm types.
But is that *always* true?
I would personally be very surprized, and consider this a false-positive.

This should at least be optional.
Not sure about the default, but setting the StrictMode should certainly disable this relaxation.

[flx]

clang-tidy/utils/TypeTraits.cpp

49

This early return now ignores the fact that the type has non-trivial copy constructors, virtual destructors etc which makes the type expensive to copy. Could we achieve the same desired outcome by adding a blacklist that allows users exclude types they deem to be not expensive?

This way they would get a warning the first time they run the check and then could disable such types.

[baloghadamsoftware]

I still wonder what true positives could we get without checking the size? No real-life types come to my mind with size of a pointer but really expensive copy operations. What could be so really expensive when copying a single machine world?

[lebedev.ri]

In D52315#1241487, @baloghadamsoftware wrote:

I still wonder what true positives could we get without checking the size? No real-life types come to my mind with size of a pointer but really expensive copy operations. What could be so really expensive when copying a single machine world?

The type is not trivially-copyable, so how do you know the user-provided copy constructors
don't treat that pointer as a pointer to a string, which they allocate and copy on each copy?

I really don't think the default should change.
I think the best solution would be simply to add type blacklists for each of these checks.

[dblaikie]

"The three checks mentioned in the Title are two noisy if the code uses intrusive smart (reference counting) pointers. 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."

I'm not quite following this comment - StringRef doesn't appear to have any connection to IntrusiveRefCntPtr so far as I can see? (I don't see anything in SymbolRef either. ProgramStateRef is an IntrusiveRefCntPtr, for sure - and has a non-trivial copy ctor - yeah, if you pass that by value unnecessarily, I think that's a real/reasonable warning?

The reason these things are passed by value a lot is when passing ownership - if the callee is moving the IntrusiveRefCntPtr into something that exceeds the duration of the function call (eg: moving it into a member function from a setter) - so the warning should take that into account. But it shouldn't rely on the sizeof of the type - that's not really a good signal here, I think.

[lebedev.ri]

In D52315#1245747, @dblaikie wrote:

...

I think this has been attempted to be superseded by D52360, although that has the same basic issue.

clang-tidy/utils/TypeTraits.cpp

 }
 
 } // namespace
 
 llvm::Optional<bool> isExpensiveToCopy(QualType Type,
                                        const ASTContext &Context) {
   if (Type->isDependentType() || Type->isIncompleteType())
     return llvm::None;
+
+  // Do not consider "expensive to copy" types not greater than a pointer
+  if (Context.getTypeSize(Type) <= Context.getTypeSize(Context.VoidPtrTy))
+    return false;
+
   return !Type.isTriviallyCopyableType(Context) &&
          !classHasTrivialCopyAndDestroy(Type) &&
          !hasDeletedCopyConstructor(Type) &&
          !Type->isObjCLifetimeType();
 }
 
 bool recordIsTriviallyDefaultConstructible(const RecordDecl &RecordDecl,
                                            const ASTContext &Context) {

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.
+which means their size is greater than a pointer and they are not trivially
+copyable or have a non-trivial copy constructor or destructor.
 
 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.
+constructor of a non-trivially-copyable type whose size is greater than the
+size of a pointer but it would suffice to obtain a const reference.
 
 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.
+which means their size is greater than a pointer and they are not trivially
+copyable or have a non-trivial copy constructor or destructor.
 
 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/Inputs/performance-unnecessary-value-param/header-fixed.h

 // struct ABC is expensive to copy and should be
 // passed as a const referece.
 struct ABC {
   ABC(const ABC&);
   int get(int) const;
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 
 int f1(int n,              const ABC& v1,   const ABC& v2); // line 9
 
 int f1(int n, ABC v1); // line 11
 
 
 
 int f2(        int n,       const ABC& v2); // line 15

test/clang-tidy/Inputs/performance-unnecessary-value-param/header.h

 // struct ABC is expensive to copy and should be
 // passed as a const referece.
 struct ABC {
   ABC(const ABC&);
   int get(int) const;
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 
-int f1(int n,              ABC v1,   ABC v2); // line 9
+int f1(int n,              const ABC& v1,   const ABC& v2); // line 9
 
 int f1(int n, ABC v1); // line 11
 
 
 
-int f2(        int n,       ABC v2); // line 15
+int f2(        int n,       const ABC& v2); // line 15

test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp

struct View {
   T end() const { return T(); }
 };
 
 struct S {
   S();
   S(const S &);
   ~S();
   S &operator=(const S &);
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 void NegativeLoopVariableNotAuto() {
   for (S S1 : View<Iterator<S>>()) {
     S* S2 = &S1;
   }
 }
 
 void PositiveTriggeredForAutoLoopVariable() {
   for (auto S1 : View<Iterator<S>>()) {
     // CHECK-MESSAGES: [[@LINE-1]]:13: warning: the loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [performance-for-range-copy]
     // CHECK-FIXES: for (const auto& S1 : View<Iterator<S>>()) {
     S* S2 = &S1;
   }
 }

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

 };
 
 struct S {
   S();
   S(const S &);
   S(const ConstructorConvertible&) {}
   ~S();
   S &operator=(const S &);
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 struct Convertible {
   operator S() const {
     return S();
   }
 };
 
+// Intrusive smart pointers are usually passed by value
+template<typename T> struct IntrusiveSmartPointerType {
+  IntrusiveSmartPointerType(const T* data = nullptr);
+  IntrusiveSmartPointerType(const IntrusiveSmartPointerType<T>& rhs);
+  const IntrusiveSmartPointerType& operator=(const T* data);
+  const IntrusiveSmartPointerType&
+  operator=(const IntrusiveSmartPointerType<T>& data);
+
+  operator T*();
+private:
+  T *Data;
+};
+
+// Wrapper for builtin types used by intrusive smart pointers
+template<typename T>
+class RefCntType {
+  unsigned count;
+  T value;
+public:
+  RefCntType(T val) : value(val) {}
+  operator T() { return value; }
+  friend class IntrusiveSmartPoitnerType;
+};
+
 void negativeConstReference() {
   for (const S &S1 : View<Iterator<S>>()) {
   }
 }
 
 void negativeUserDefinedConversion() {
   Convertible C[0];
   for (const S &S1 : C) {
struct Mutable {
   }
   Mutable& operator[](int I) {
     return *this;
   }
   bool operator==(const Mutable &Other) const {
     return true;
   }
   ~Mutable() {}
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 struct Point {
   ~Point() {}
-  int x, y;
+  long x, y;
 };
 
 Mutable& operator<<(Mutable &Out, bool B) {
   Out.setBool(B);
   return Out;
 }
 
 bool operator!=(const Mutable& M1, const Mutable& M2) {
for (auto ConstOperatorInvokee : View<Iterator<Mutable>>()) {
     bool result = ConstOperatorInvokee != Mutable();
   }
 }
 
 void IgnoreLoopVariableNotUsedInLoopBody() {
   for (auto _ : View<Iterator<S>>()) {
   }
 }
+
+void NegativeIntrusiveSmartPointer() {
+  int Sum = 0;
+  for (auto I : View<Iterator<IntrusiveSmartPointerType<RefCntType<int>>>>()) {
+    Sum += *I;
+  }
+}

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

 // RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
 
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
   void nonConstMethod();
   bool constMethod() const;
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 struct TrivialToCopyType {
   const TrivialToCopyType &reference() const;
 };
 
 struct WeirdCopyCtorType {
   WeirdCopyCtorType();
   WeirdCopyCtorType(const WeirdCopyCtorType &w, bool oh_yes = true);
 
   void nonConstMethod();
   bool constMethod() const;
+private:
+  // Greater than a pointer
+  long Data1, Data2;
+};
+
+// Intrusive smart pointers are usually passed by value
+template<typename T> struct IntrusiveSmartPointerType {
+  IntrusiveSmartPointerType(const T* data);
+  IntrusiveSmartPointerType(const IntrusiveSmartPointerType<T>& rhs);
+  const IntrusiveSmartPointerType& operator=(const T* data);
+  const IntrusiveSmartPointerType&
+  operator=(const IntrusiveSmartPointerType<T>& data);
+
+  operator T*();
+private:
+  T *Data;
+};
+
+// Wrapper for builtin types used by intrusive smart pointers
+template<typename T>
+class RefCntType {
+  unsigned count;
+  T value;
+public:
+  RefCntType(T val) : value(val) {}
+  operator T() { return value; }
+  friend class IntrusiveSmartPoitnerType;
 };
 
 ExpensiveToCopyType global_expensive_to_copy_type;
 
 const ExpensiveToCopyType &ExpensiveTypeReference();
 const TrivialToCopyType &TrivialTypeReference();
+template<typename T>
+const IntrusiveSmartPointerType<RefCntType<T>>
+&IntrusiveSmartPointerTypeReference();
 
 void mutate(ExpensiveToCopyType &);
 void mutate(ExpensiveToCopyType *);
 void useAsConstPointer(const ExpensiveToCopyType *);
 void useAsConstReference(const ExpensiveToCopyType &);
 void useByValue(ExpensiveToCopyType);
 
 void PositiveFunctionCall() {
public:
   public:
     void operator++();
     Element operator*();
     bool operator!=(const Iterator &);
     WeirdCopyCtorType c;
   };
   const Iterator &begin() const;
   const Iterator &end() const;
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 void implicitVarFalsePositive() {
   for (const Element &E : Container()) {
   }
 }
+
+void NegativeIntrusiveSmartPointerType() {
+  IntrusiveSmartPointerType<RefCntType<int>> N =
+    IntrusiveSmartPointerTypeReference<int>();
+}

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

 // RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -- -fdelayed-template-parsing
 
 struct ExpensiveToCopyType {
   const ExpensiveToCopyType & constReference() const {
     return *this;
   }
   void nonConstMethod();
   virtual ~ExpensiveToCopyType();
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 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&);
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 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) {
 }

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

 // RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
 
 // CHECK-FIXES: #include <utility>
 
 struct ExpensiveToCopyType {
   const ExpensiveToCopyType & constReference() const {
     return *this;
   }
   void nonConstMethod();
   virtual ~ExpensiveToCopyType();
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 void mutate(ExpensiveToCopyType &);
 void mutate(ExpensiveToCopyType *);
 void useAsConstReference(const ExpensiveToCopyType &);
 void useByValue(ExpensiveToCopyType);
 
 template <class T> class Vector {
  public:
   using iterator = T*;
   using const_iterator = const T*;
 
   Vector(const Vector&);
   Vector& operator=(const Vector&);
 
   iterator begin();
   iterator end();
   const_iterator begin() const;
   const_iterator end() const;
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 // 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&);
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 struct MoveOnlyType {
   MoveOnlyType(const MoveOnlyType &) = delete;
   MoveOnlyType(MoveOnlyType &&) = default;
   ~MoveOnlyType();
   void constMethod() const;
+private:
+  // Greater than a pointer
+  long Data1, Data2;
 };
 
 struct ExpensiveMovableType {
   ExpensiveMovableType();
   ExpensiveMovableType(ExpensiveMovableType &&);
   ExpensiveMovableType(const ExpensiveMovableType &) = default;
   ExpensiveMovableType &operator=(const ExpensiveMovableType &) = default;
   ExpensiveMovableType &operator=(ExpensiveMovableType &&);
   ~ExpensiveMovableType();
+private:
+  // Greater than a pointer
+  long Data1, Data2;
+};
+
+// Intrusive smart pointers are usually passed by value
+template<typename T> struct IntrusiveSmartPointerType {
+  IntrusiveSmartPointerType(const T* data);
+  IntrusiveSmartPointerType(const IntrusiveSmartPointerType<T>& rhs);
+  const IntrusiveSmartPointerType& operator=(const T* data);
+  const IntrusiveSmartPointerType&
+  operator=(const IntrusiveSmartPointerType<T>& data);
+
+  operator T*();
+private:
+  T *Data;
+};
+
+// Wrapper for builtin types used by intrusive smart pointers
+template<typename T>
+class RefCntType {
+  unsigned count;
+  T value;
+public:
+  RefCntType(T val) : value(val) {}
+  operator T() { return value; }
+  friend class IntrusiveSmartPoitnerType;
 };
 
 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) {
 }

 }
 
 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();
 }
+
+// Do not warn for intrusive smart pointers which are usually passed by value
+void negativeIntrusiveSmartPointer(const IntrusiveSmartPointerType<RefCntType<int>> Obj);
+
+void negativeIntrusiveSmartPointer(const IntrusiveSmartPointerType<RefCntType<int>> Obj) {
+}
+