baloghadamsoftware created this revision.
baloghadamsoftware added reviewers: alexfh, aaron.ballman, flx.
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.
https://reviews.llvm.org/D52315
Files:
clang-tidy/utils/TypeTraits.cpp
docs/clang-tidy/checks/performance-for-range-copy.rst
docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
docs/clang-tidy/checks/performance-unnecessary-value-param.rst
test/clang-tidy/Inputs/performance-unnecessary-value-param/header-fixed.h
test/clang-tidy/Inputs/performance-unnecessary-value-param/header.h
test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
test/clang-tidy/performance-for-range-copy.cpp
test/clang-tidy/performance-unnecessary-copy-initialization.cpp
test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
test/clang-tidy/performance-unnecessary-value-param.cpp
Index: test/clang-tidy/performance-unnecessary-value-param.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-value-param.cpp
+++ test/clang-tidy/performance-unnecessary-value-param.cpp
@@ -8,6 +8,9 @@
}
void nonConstMethod();
virtual ~ExpensiveToCopyType();
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
void mutate(ExpensiveToCopyType &);
@@ -27,6 +30,9 @@
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
@@ -37,13 +43,19 @@
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 {
@@ -53,6 +65,33 @@
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);
@@ -381,3 +420,10 @@
// 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) {
+}
+
Index: test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
+++ test/clang-tidy/performance-unnecessary-value-param-delayed.cpp
@@ -6,6 +6,9 @@
}
void nonConstMethod();
virtual ~ExpensiveToCopyType();
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
void mutate(ExpensiveToCopyType &);
@@ -21,6 +24,9 @@
SomewhatTrivial(const SomewhatTrivial&) = default;
~SomewhatTrivial() = default;
SomewhatTrivial& operator=(const SomewhatTrivial&);
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
Index: test/clang-tidy/performance-unnecessary-copy-initialization.cpp
===================================================================
--- test/clang-tidy/performance-unnecessary-copy-initialization.cpp
+++ test/clang-tidy/performance-unnecessary-copy-initialization.cpp
@@ -6,6 +6,9 @@
const ExpensiveToCopyType &reference() const;
void nonConstMethod();
bool constMethod() const;
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
struct TrivialToCopyType {
@@ -18,12 +21,42 @@
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 *);
@@ -381,9 +414,17 @@
};
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>();
+}
Index: test/clang-tidy/performance-for-range-copy.cpp
===================================================================
--- test/clang-tidy/performance-for-range-copy.cpp
+++ test/clang-tidy/performance-for-range-copy.cpp
@@ -44,14 +44,41 @@
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>>()) {
}
@@ -120,11 +147,14 @@
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) {
@@ -270,3 +300,10 @@
for (auto _ : View<Iterator<S>>()) {
}
}
+
+void NegativeIntrusiveSmartPointer() {
+ int Sum = 0;
+ for (auto I : View<Iterator<IntrusiveSmartPointerType<RefCntType<int>>>>()) {
+ Sum += *I;
+ }
+}
Index: test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
===================================================================
--- test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
+++ test/clang-tidy/performance-for-range-copy-warn-on-all-auto-copies.cpp
@@ -22,6 +22,9 @@
S(const S &);
~S();
S &operator=(const S &);
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
void NegativeLoopVariableNotAuto() {
Index: test/clang-tidy/Inputs/performance-unnecessary-value-param/header.h
===================================================================
--- test/clang-tidy/Inputs/performance-unnecessary-value-param/header.h
+++ test/clang-tidy/Inputs/performance-unnecessary-value-param/header.h
@@ -3,13 +3,16 @@
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
Index: test/clang-tidy/Inputs/performance-unnecessary-value-param/header-fixed.h
===================================================================
--- test/clang-tidy/Inputs/performance-unnecessary-value-param/header-fixed.h
+++ test/clang-tidy/Inputs/performance-unnecessary-value-param/header-fixed.h
@@ -3,6 +3,9 @@
struct ABC {
ABC(const ABC&);
int get(int) const;
+private:
+ // Greater than a pointer
+ long Data1, Data2;
};
Index: docs/clang-tidy/checks/performance-unnecessary-value-param.rst
===================================================================
--- docs/clang-tidy/checks/performance-unnecessary-value-param.rst
+++ docs/clang-tidy/checks/performance-unnecessary-value-param.rst
@@ -7,8 +7,8 @@
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:
Index: docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
===================================================================
--- docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
+++ docs/clang-tidy/checks/performance-unnecessary-copy-initialization.rst
@@ -4,8 +4,8 @@
===========================================
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
Index: docs/clang-tidy/checks/performance-for-range-copy.rst
===================================================================
--- docs/clang-tidy/checks/performance-for-range-copy.rst
+++ docs/clang-tidy/checks/performance-for-range-copy.rst
@@ -7,8 +7,8 @@
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:
Index: clang-tidy/utils/TypeTraits.cpp
===================================================================
--- clang-tidy/utils/TypeTraits.cpp
+++ clang-tidy/utils/TypeTraits.cpp
@@ -43,6 +43,11 @@
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) &&
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
