Fix interaction between RetainPtr<T> and transparent comparisons.
The key is providing an implicit operator T* so that we can make
comparisons in terms of raw pointers in all cases. Unfortunately,
this now allows passing RetainPtr<T> as T* without an explicit
Get(), which is cleaner but will not catch the use of unretained
references.
-- remove now-redundant comparison functions.
-- kludge around some ambiguity in CPDF_DictionaryLocker
Change-Id: Ib06f665361f4efa514c6f028eb24af114f7506dc
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/98394
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/parser/cpdf_dictionary_unittest.cpp b/core/fpdfapi/parser/cpdf_dictionary_unittest.cpp
index e0a641f..73bd8c4 100644
--- a/core/fpdfapi/parser/cpdf_dictionary_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_dictionary_unittest.cpp
@@ -18,7 +18,7 @@
dict->SetNewFor<CPDF_Stream>("the-stream");
dict->SetNewFor<CPDF_Number>("the-number", 42);
- CPDF_DictionaryLocker locked_dict(std::move(dict));
+ CPDF_DictionaryLocker locked_dict(dict.Get());
auto it = locked_dict.begin();
EXPECT_NE(it, locked_dict.end());
EXPECT_EQ(it->first, ByteString("the-array"));
diff --git a/core/fxcrt/retain_ptr.h b/core/fxcrt/retain_ptr.h
index bbd4bce..fb1c95b 100644
--- a/core/fxcrt/retain_ptr.h
+++ b/core/fxcrt/retain_ptr.h
@@ -71,8 +71,9 @@
m_pObj.reset(obj);
}
- explicit operator T*() const { return Get(); }
- T* Get() const { return m_pObj.get(); }
+ operator T*() const noexcept { return Get(); }
+ T* Get() const noexcept { return m_pObj.get(); }
+
UnownedPtr<T> BackPointer() const { return UnownedPtr<T>(Get()); }
void Swap(RetainPtr& that) { m_pObj.swap(that.m_pObj); }
@@ -150,16 +151,6 @@
mutable intptr_t m_nRefCount = 0;
};
-template <typename T, typename U>
-inline bool operator==(const U* lhs, const RetainPtr<T>& rhs) {
- return rhs == lhs;
-}
-
-template <typename T, typename U>
-inline bool operator!=(const U* lhs, const RetainPtr<T>& rhs) {
- return rhs != lhs;
-}
-
} // namespace fxcrt
using fxcrt::ReleaseDeleter;
diff --git a/core/fxcrt/retain_ptr_unittest.cpp b/core/fxcrt/retain_ptr_unittest.cpp
index ffb98b1..e3ca7e4 100644
--- a/core/fxcrt/retain_ptr_unittest.cpp
+++ b/core/fxcrt/retain_ptr_unittest.cpp
@@ -4,6 +4,7 @@
#include "core/fxcrt/retain_ptr.h"
+#include <functional>
#include <set>
#include <utility>
#include <vector>
@@ -12,6 +13,17 @@
#include "testing/pseudo_retainable.h"
#include "third_party/base/containers/contains.h"
+namespace {
+
+template <typename T, typename C = std::less<T>>
+class NoLinearSearchSet : public std::set<T, C> {
+ public:
+ typename std::set<T, C>::iterator begin() noexcept = delete;
+ typename std::set<T, C>::const_iterator cbegin() const noexcept = delete;
+};
+
+} // namespace
+
namespace fxcrt {
namespace {
@@ -421,21 +433,53 @@
// RetainPtrs for containers that use find().
PseudoRetainable obj1;
PseudoRetainable obj2;
- NoLinearSearchSet<const PseudoRetainable*, std::less<>> the_set;
- the_set.insert(&obj1);
- EXPECT_TRUE(pdfium::Contains(the_set, &obj1));
- EXPECT_FALSE(pdfium::Contains(the_set, &obj2));
-
RetainPtr<PseudoRetainable> ptr1(&obj1);
RetainPtr<PseudoRetainable> ptr2(&obj2);
- // TODO(tsepez): remove Get() after fixing transparent compare for RetainPtr.
- EXPECT_TRUE(pdfium::Contains(the_set, ptr1.Get()));
- EXPECT_FALSE(pdfium::Contains(the_set, ptr2.Get()));
-
RetainPtr<const PseudoRetainable> const_ptr1(&obj1);
RetainPtr<const PseudoRetainable> const_ptr2(&obj2);
- EXPECT_TRUE(pdfium::Contains(the_set, const_ptr1.Get()));
- EXPECT_FALSE(pdfium::Contains(the_set, const_ptr2.Get()));
+ NoLinearSearchSet<RetainPtr<const PseudoRetainable>, std::less<>> the_set;
+
+ // Intially, two smart pointers to each object.
+ EXPECT_EQ(2, obj1.retain_count());
+ EXPECT_EQ(0, obj1.release_count());
+ EXPECT_EQ(2, obj2.retain_count());
+ EXPECT_EQ(0, obj2.release_count());
+
+ // Passed by const-ref, increment on copy into set's data structure.
+ the_set.insert(const_ptr1);
+ EXPECT_EQ(3, obj1.retain_count());
+ EXPECT_EQ(0, obj1.release_count());
+ EXPECT_EQ(2, obj2.retain_count());
+ EXPECT_EQ(0, obj2.release_count());
+
+ // None of the following should cause any churn.
+ EXPECT_NE(the_set.end(), the_set.find(&obj1));
+ EXPECT_EQ(the_set.end(), the_set.find(&obj2));
+ EXPECT_TRUE(pdfium::Contains(the_set, &obj1));
+ EXPECT_FALSE(pdfium::Contains(the_set, &obj2));
+ EXPECT_EQ(3, obj1.retain_count());
+ EXPECT_EQ(0, obj1.release_count());
+ EXPECT_EQ(2, obj2.retain_count());
+ EXPECT_EQ(0, obj2.release_count());
+
+ EXPECT_NE(the_set.end(), the_set.find(const_ptr1));
+ EXPECT_EQ(the_set.end(), the_set.find(const_ptr2));
+ EXPECT_TRUE(pdfium::Contains(the_set, const_ptr1));
+ EXPECT_FALSE(pdfium::Contains(the_set, const_ptr2));
+ EXPECT_EQ(3, obj1.retain_count());
+ EXPECT_EQ(0, obj1.release_count());
+ EXPECT_EQ(2, obj2.retain_count());
+ EXPECT_EQ(0, obj2.release_count());
+
+ // These involve const-removing conversions which seem to churn.
+ EXPECT_NE(the_set.end(), the_set.find(ptr1));
+ EXPECT_EQ(the_set.end(), the_set.find(ptr2));
+ EXPECT_TRUE(pdfium::Contains(the_set, ptr1));
+ EXPECT_FALSE(pdfium::Contains(the_set, ptr2));
+ EXPECT_EQ(5, obj1.retain_count());
+ EXPECT_EQ(2, obj1.release_count());
+ EXPECT_EQ(4, obj2.retain_count());
+ EXPECT_EQ(2, obj2.release_count());
}
TEST(RetainPtr, VectorContains) {
