Make UnownedPtr<> interoperate more transparently with raw pointers.
There is now interest in using some sort of unowned ptr variant
beyond PDFium, but concerns exist about minimizing the amount of
code that needs to change during the adoption of it. Demonstrate
the effects in PDFium of adopting such a new interface:
-- Add an implicit operator T*() which is preferred to .Get().
-- The existing operator=() will become preferred over .Reset().
-- Then remove comparison operators which become ambiguous once
demotion to raw pointers is implicit.
Also update the node iterator template, because it is instantiated
against both RetainPtr and UnownedPtr. This is possible because
both of these have the same "shaped" API. In turn, update RetainPtr
to match the new shape:
-- Add an explicit operator T*() (not implicit, want to limit use)
-- Allow assignment from the underlying type in place of .Reset().
Change-Id: I50828f50b3debb0b1b9ddd494cb640048933d92b
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/68591
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fxcrt/retain_ptr.h b/core/fxcrt/retain_ptr.h
index 916a12e..1bbd96a 100644
--- a/core/fxcrt/retain_ptr.h
+++ b/core/fxcrt/retain_ptr.h
@@ -53,6 +53,7 @@
m_pObj.reset(obj);
}
+ explicit operator T*() const { return Get(); }
T* Get() const { return m_pObj.get(); }
UnownedPtr<T> BackPointer() const { return UnownedPtr<T>(Get()); }
void Swap(RetainPtr& that) { m_pObj.swap(that.m_pObj); }
@@ -73,8 +74,11 @@
return *this;
}
- // Assigment from raw pointers is intentially not provided to make
- // reference count churn more visible where possible.
+ // Use sparingly, may produce reference count churn.
+ RetainPtr& operator=(T* that) {
+ Reset(that);
+ return *this;
+ }
bool operator==(const RetainPtr& that) const { return Get() == that.Get(); }
bool operator!=(const RetainPtr& that) const { return !(*this == that); }
diff --git a/core/fxcrt/retain_ptr_unittest.cpp b/core/fxcrt/retain_ptr_unittest.cpp
index 47277df..f7bbfdd 100644
--- a/core/fxcrt/retain_ptr_unittest.cpp
+++ b/core/fxcrt/retain_ptr_unittest.cpp
@@ -166,11 +166,17 @@
EXPECT_EQ(2, obj.retain_count());
EXPECT_EQ(0, obj.release_count());
}
- EXPECT_EQ(2, obj.retain_count());
- EXPECT_EQ(1, obj.release_count());
+ {
+ RetainPtr<PseudoRetainable> ptr2;
+ ptr2 = ptr.Get(); // Test assignment from underlying type.
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(1, obj.release_count());
+ }
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(2, obj.release_count());
}
- EXPECT_EQ(2, obj.retain_count());
- EXPECT_EQ(2, obj.release_count());
+ EXPECT_EQ(3, obj.retain_count());
+ EXPECT_EQ(3, obj.release_count());
}
TEST(RetainPtr, MoveAssign) {
diff --git a/core/fxcrt/unowned_ptr.h b/core/fxcrt/unowned_ptr.h
index f7ff480..1210248 100644
--- a/core/fxcrt/unowned_ptr.h
+++ b/core/fxcrt/unowned_ptr.h
@@ -91,16 +91,7 @@
return std::less<T*>()(Get(), that.Get());
}
- template <typename U>
- bool operator==(const U* that) const {
- return Get() == that;
- }
-
- template <typename U>
- bool operator!=(const U* that) const {
- return !(*this == that);
- }
-
+ operator T*() const noexcept { return Get(); }
T* Get() const noexcept { return m_pObj; }
T* Release() {
@@ -133,16 +124,6 @@
T* m_pObj = nullptr;
};
-template <typename T, typename U>
-inline bool operator==(const U* lhs, const UnownedPtr<T>& rhs) {
- return rhs == lhs;
-}
-
-template <typename T, typename U>
-inline bool operator!=(const U* lhs, const UnownedPtr<T>& rhs) {
- return rhs != lhs;
-}
-
} // namespace fxcrt
using fxcrt::UnownedPtr;
diff --git a/xfa/fxfa/parser/cxfa_nodeiteratortemplate.h b/xfa/fxfa/parser/cxfa_nodeiteratortemplate.h
index 8803efa..7887d86 100644
--- a/xfa/fxfa/parser/cxfa_nodeiteratortemplate.h
+++ b/xfa/fxfa/parser/cxfa_nodeiteratortemplate.h
@@ -17,8 +17,8 @@
explicit CXFA_NodeIteratorTemplate(NodeType* pRoot)
: m_pRoot(pRoot), m_pCurrent(pRoot) {}
- NodeType* GetRoot() const { return m_pRoot.Get(); }
- NodeType* GetCurrent() const { return m_pCurrent.Get(); }
+ NodeType* GetRoot() const { return static_cast<NodeType*>(m_pRoot); }
+ NodeType* GetCurrent() const { return static_cast<NodeType*>(m_pCurrent); }
void Reset() { m_pCurrent = m_pRoot; }
bool SetCurrent(NodeType* pNode) {
@@ -26,7 +26,7 @@
m_pCurrent = nullptr;
return false;
}
- m_pCurrent.Reset(pNode);
+ m_pCurrent = pNode;
return true;
}
@@ -34,26 +34,28 @@
if (!m_pRoot)
return nullptr;
if (!m_pCurrent) {
- m_pCurrent.Reset(LastDescendant(m_pRoot.Get()));
- return m_pCurrent.Get();
+ m_pCurrent = LastDescendant(static_cast<NodeType*>(m_pRoot));
+ return static_cast<NodeType*>(m_pCurrent);
}
- NodeType* pSibling = PreviousSiblingWithinSubtree(m_pCurrent.Get());
+ NodeType* pSibling =
+ PreviousSiblingWithinSubtree(static_cast<NodeType*>(m_pCurrent));
if (pSibling) {
- m_pCurrent.Reset(LastDescendant(pSibling));
- return m_pCurrent.Get();
+ m_pCurrent = LastDescendant(pSibling);
+ return static_cast<NodeType*>(m_pCurrent);
}
- NodeType* pParent = ParentWithinSubtree(m_pCurrent.Get());
+ NodeType* pParent = ParentWithinSubtree(static_cast<NodeType*>(m_pCurrent));
if (pParent)
- m_pCurrent.Reset(pParent);
+ m_pCurrent = pParent;
return pParent;
}
NodeType* MoveToNext() {
if (!m_pRoot || !m_pCurrent)
return nullptr;
- NodeType* pChild = TraverseStrategy::GetFirstChild(m_pCurrent.Get());
+ NodeType* pChild =
+ TraverseStrategy::GetFirstChild(static_cast<NodeType*>(m_pCurrent));
if (pChild) {
- m_pCurrent.Reset(pChild);
+ m_pCurrent = pChild;
return pChild;
}
return SkipChildrenAndMoveToNext();
@@ -62,11 +64,11 @@
NodeType* SkipChildrenAndMoveToNext() {
if (!m_pRoot)
return nullptr;
- NodeType* pNode = m_pCurrent.Get();
+ NodeType* pNode = static_cast<NodeType*>(m_pCurrent);
while (pNode) {
NodeType* pSibling = NextSiblingWithinSubtree(pNode);
if (pSibling) {
- m_pCurrent.Reset(pSibling);
+ m_pCurrent = pSibling;
return pSibling;
}
pNode = ParentWithinSubtree(pNode);
