From 81b224669647c9dd3c5ea2e013ee3cc109744bb8 Mon Sep 17 00:00:00 2001 From: tsepez Date: Wed, 23 Nov 2016 14:20:19 -0800 Subject: Add CFX_MaybeOwned<> template. This will allow us to get rid of more .release()s of unique_ptrs, as shown by the changed cpdf_colorspace usage. Review-Url: https://codereview.chromium.org/2526903002 --- BUILD.gn | 2 + core/fpdfapi/page/cpdf_colorspace.cpp | 21 ++-- core/fxcrt/cfx_maybe_owned.h | 88 ++++++++++++++++ core/fxcrt/cfx_maybe_owned_unittest.cpp | 179 ++++++++++++++++++++++++++++++++ 4 files changed, 277 insertions(+), 13 deletions(-) create mode 100644 core/fxcrt/cfx_maybe_owned.h create mode 100644 core/fxcrt/cfx_maybe_owned_unittest.cpp diff --git a/BUILD.gn b/BUILD.gn index 29572e72c5..37ae9e0580 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -703,6 +703,7 @@ config("fxge_warnings") { static_library("fxcrt") { sources = [ + "core/fxcrt/cfx_maybe_owned.h", "core/fxcrt/cfx_observable.h", "core/fxcrt/cfx_retain_ptr.h", "core/fxcrt/cfx_shared_copy_on_write.h", @@ -1734,6 +1735,7 @@ test("pdfium_unittests") { "core/fpdftext/fpdf_text_int_unittest.cpp", "core/fxcodec/codec/fx_codec_jpx_unittest.cpp", "core/fxcodec/jbig2/JBig2_Image_unittest.cpp", + "core/fxcrt/cfx_maybe_owned_unittest.cpp", "core/fxcrt/cfx_observable_unittest.cpp", "core/fxcrt/cfx_retain_ptr_unittest.cpp", "core/fxcrt/cfx_shared_copy_on_write_unittest.cpp", diff --git a/core/fpdfapi/page/cpdf_colorspace.cpp b/core/fpdfapi/page/cpdf_colorspace.cpp index b1675d87a7..6cd0075e8e 100644 --- a/core/fpdfapi/page/cpdf_colorspace.cpp +++ b/core/fpdfapi/page/cpdf_colorspace.cpp @@ -7,6 +7,7 @@ #include "core/fpdfapi/page/cpdf_colorspace.h" #include +#include #include "core/fpdfapi/cpdf_modulemgr.h" #include "core/fpdfapi/page/cpdf_docpagedata.h" @@ -20,6 +21,8 @@ #include "core/fpdfapi/parser/cpdf_stream_acc.h" #include "core/fpdfapi/parser/cpdf_string.h" #include "core/fxcodec/fx_codec.h" +#include "core/fxcrt/cfx_maybe_owned.h" +#include "core/fxcrt/fx_memory.h" namespace { @@ -175,11 +178,10 @@ class CPDF_ICCBasedCS : public CPDF_ColorSpace { int image_height, bool bTransMask = false) const override; - CPDF_ColorSpace* m_pAlterCS; + CFX_MaybeOwned m_pAlterCS; CPDF_IccProfile* m_pProfile; uint8_t* m_pCache; FX_FLOAT* m_pRanges; - bool m_bOwn; }; class CPDF_IndexedCS : public CPDF_ColorSpace { @@ -815,17 +817,13 @@ void CPDF_LabCS::TranslateImageLine(uint8_t* pDestBuf, CPDF_ICCBasedCS::CPDF_ICCBasedCS(CPDF_Document* pDoc) : CPDF_ColorSpace(pDoc, PDFCS_ICCBASED, 0), - m_pAlterCS(nullptr), m_pProfile(nullptr), m_pCache(nullptr), - m_pRanges(nullptr), - m_bOwn(false) {} + m_pRanges(nullptr) {} CPDF_ICCBasedCS::~CPDF_ICCBasedCS() { FX_Free(m_pCache); FX_Free(m_pRanges); - if (m_pAlterCS && m_bOwn) - m_pAlterCS->Release(); if (m_pProfile && m_pDocument) m_pDocument->GetPageData()->ReleaseIccProfile(m_pProfile); } @@ -852,8 +850,7 @@ bool CPDF_ICCBasedCS::v_Load(CPDF_Document* pDoc, CPDF_Array* pArray) { if (m_nComponents == 0) { // NO valid ICC profile if (pAlterCS->CountComponents() > 0) { // Use Alternative colorspace m_nComponents = pAlterCS->CountComponents(); - m_pAlterCS = pAlterCS.release(); - m_bOwn = true; + m_pAlterCS = std::move(pAlterCS); } else { // No valid alternative colorspace int32_t nDictComponents = pDict ? pDict->GetIntegerFor("N") : 0; if (nDictComponents != 1 && nDictComponents != 3 && @@ -863,10 +860,8 @@ bool CPDF_ICCBasedCS::v_Load(CPDF_Document* pDoc, CPDF_Array* pArray) { m_nComponents = nDictComponents; } } else { // Using sRGB - if (pAlterCS->CountComponents() == m_nComponents) { - m_pAlterCS = pAlterCS.release(); - m_bOwn = true; - } + if (pAlterCS->CountComponents() == m_nComponents) + m_pAlterCS = std::move(pAlterCS); } } } diff --git a/core/fxcrt/cfx_maybe_owned.h b/core/fxcrt/cfx_maybe_owned.h new file mode 100644 index 0000000000..76bd580e28 --- /dev/null +++ b/core/fxcrt/cfx_maybe_owned.h @@ -0,0 +1,88 @@ +// Copyright 2016 PDFium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CORE_FXCRT_CFX_MAYBE_OWNED_H_ +#define CORE_FXCRT_CFX_MAYBE_OWNED_H_ + +#include +#include +#include + +#include "core/fxcrt/fx_memory.h" +#include "core/fxcrt/fx_system.h" + +// A template that can hold either owned or unowned references, and cleans up +// appropriately. Possibly the most pernicious anti-pattern imaginable, but +// it crops up throughout the codebase due to a desire to avoid copying-in +// objects or data. +template > +class CFX_MaybeOwned { + public: + CFX_MaybeOwned() : m_pObj(nullptr) {} + explicit CFX_MaybeOwned(T* ptr) : m_pObj(ptr) {} + explicit CFX_MaybeOwned(std::unique_ptr ptr) + : m_pOwnedObj(std::move(ptr)), m_pObj(m_pOwnedObj.get()) {} + + CFX_MaybeOwned(const CFX_MaybeOwned& that) = delete; + CFX_MaybeOwned(CFX_MaybeOwned&& that) + : m_pOwnedObj(that.m_pOwnedObj.release()), m_pObj(that.m_pObj) { + that.m_pObj = nullptr; + } + + void Reset(std::unique_ptr ptr) { + m_pOwnedObj = std::move(ptr); + m_pObj = m_pOwnedObj.get(); + } + void Reset(T* ptr = nullptr) { + m_pOwnedObj.reset(); + m_pObj = ptr; + } + + bool IsOwned() const { return !!m_pOwnedObj; } + T* Get() const { return m_pObj; } + std::unique_ptr Release() { + ASSERT(IsOwned()); + return std::move(m_pOwnedObj); + } + + CFX_MaybeOwned& operator=(const CFX_MaybeOwned& that) = delete; + CFX_MaybeOwned& operator=(CFX_MaybeOwned&& that) { + m_pOwnedObj = std::move(that.m_pOwnedObj); + m_pObj = that.m_pObj; + that.m_pObj = nullptr; + return *this; + } + CFX_MaybeOwned& operator=(T* ptr) { + Reset(ptr); + return *this; + } + CFX_MaybeOwned& operator=(std::unique_ptr ptr) { + Reset(std::move(ptr)); + return *this; + } + + bool operator==(const CFX_MaybeOwned& that) const { + return Get() == that.Get(); + } + bool operator==(const std::unique_ptr& ptr) const { + return Get() == ptr.get(); + } + bool operator==(T* ptr) const { return Get() == ptr; } + + bool operator!=(const CFX_MaybeOwned& that) const { return !(*this == that); } + bool operator!=(const std::unique_ptr ptr) const { + return !(*this == ptr); + } + bool operator!=(T* ptr) const { return !(*this == ptr); } + + explicit operator bool() const { return !!m_pObj; } + T& operator*() const { return *m_pObj; } + T* operator->() const { return m_pObj; } + + private: + std::unique_ptr m_pOwnedObj; + T* m_pObj; +}; + +#endif // CORE_FXCRT_CFX_MAYBE_OWNED_H_ diff --git a/core/fxcrt/cfx_maybe_owned_unittest.cpp b/core/fxcrt/cfx_maybe_owned_unittest.cpp new file mode 100644 index 0000000000..8f513a358d --- /dev/null +++ b/core/fxcrt/cfx_maybe_owned_unittest.cpp @@ -0,0 +1,179 @@ +// Copyright 2016 PDFium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "core/fxcrt/cfx_maybe_owned.h" + +#include +#include + +#include "core/fxcrt/fx_memory.h" +#include "testing/fx_string_testhelpers.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "third_party/base/ptr_util.h" + +namespace { + +class PseudoDeletable { + public: + explicit PseudoDeletable(int id, int* count_location) + : id_(id), count_location_(count_location) {} + ~PseudoDeletable() { ++(*count_location_); } + int GetID() const { return id_; } + + private: + int id_; + int* count_location_; +}; + +} // namespace + +TEST(fxcrt, MaybeOwnedNull) { + CFX_MaybeOwned ptr1; + EXPECT_FALSE(ptr1.IsOwned()); + EXPECT_FALSE(ptr1); + EXPECT_EQ(nullptr, ptr1.Get()); + + CFX_MaybeOwned ptr2; + EXPECT_TRUE(ptr1 == ptr2); + EXPECT_FALSE(ptr1 != ptr2); +} + +TEST(fxcrt, MaybeOwnedNotOwned) { + int delete_count = 0; + PseudoDeletable thing1(100, &delete_count); + { + CFX_MaybeOwned ptr(&thing1); + EXPECT_FALSE(ptr.IsOwned()); + EXPECT_EQ(ptr.Get(), &thing1); + EXPECT_EQ(100, ptr->GetID()); + EXPECT_TRUE(ptr == &thing1); + EXPECT_FALSE(ptr != &thing1); + + CFX_MaybeOwned empty; + EXPECT_FALSE(ptr == empty); + EXPECT_TRUE(ptr != empty); + } + EXPECT_EQ(0, delete_count); + + delete_count = 0; + PseudoDeletable thing2(200, &delete_count); + { + CFX_MaybeOwned ptr(&thing1); + ptr = &thing2; + EXPECT_FALSE(ptr.IsOwned()); + EXPECT_EQ(ptr.Get(), &thing2); + EXPECT_EQ(200, ptr->GetID()); + } + EXPECT_EQ(0, delete_count); + + delete_count = 0; + int owned_delete_count = 0; + { + CFX_MaybeOwned ptr(&thing1); + EXPECT_EQ(100, ptr->GetID()); + ptr = pdfium::MakeUnique(300, &owned_delete_count); + EXPECT_TRUE(ptr.IsOwned()); + EXPECT_EQ(300, ptr->GetID()); + } + EXPECT_EQ(0, delete_count); + EXPECT_EQ(1, owned_delete_count); +} + +TEST(fxcrt, MaybeOwnedOwned) { + int delete_count = 0; + { + CFX_MaybeOwned ptr( + pdfium::MakeUnique(100, &delete_count)); + EXPECT_TRUE(ptr.IsOwned()); + EXPECT_EQ(100, ptr->GetID()); + + CFX_MaybeOwned empty; + EXPECT_FALSE(ptr == empty); + EXPECT_TRUE(ptr != empty); + } + EXPECT_EQ(1, delete_count); + + delete_count = 0; + { + CFX_MaybeOwned ptr( + pdfium::MakeUnique(200, &delete_count)); + ptr = pdfium::MakeUnique(300, &delete_count); + EXPECT_TRUE(ptr.IsOwned()); + EXPECT_EQ(300, ptr->GetID()); + EXPECT_EQ(1, delete_count); + } + EXPECT_EQ(2, delete_count); + + delete_count = 0; + int unowned_delete_count = 0; + PseudoDeletable thing2(400, &unowned_delete_count); + { + CFX_MaybeOwned ptr( + pdfium::MakeUnique(500, &delete_count)); + ptr = &thing2; + EXPECT_FALSE(ptr.IsOwned()); + EXPECT_EQ(400, ptr->GetID()); + EXPECT_EQ(1, delete_count); + EXPECT_EQ(0, unowned_delete_count); + } + EXPECT_EQ(1, delete_count); + EXPECT_EQ(0, unowned_delete_count); +} + +TEST(fxcrt, MaybeOwnedRelease) { + int delete_count = 0; + { + std::unique_ptr stolen; + { + CFX_MaybeOwned ptr( + pdfium::MakeUnique(100, &delete_count)); + EXPECT_TRUE(ptr.IsOwned()); + stolen = ptr.Release(); + EXPECT_FALSE(ptr.IsOwned()); + EXPECT_EQ(ptr, stolen); + EXPECT_EQ(0, delete_count); + } + EXPECT_EQ(0, delete_count); + } + EXPECT_EQ(1, delete_count); +} + +TEST(fxcrt, MaybeOwnedMove) { + int delete_count = 0; + PseudoDeletable thing1(100, &delete_count); + { + CFX_MaybeOwned ptr1(&thing1); + CFX_MaybeOwned ptr2( + pdfium::MakeUnique(200, &delete_count)); + EXPECT_FALSE(ptr1.IsOwned()); + EXPECT_TRUE(ptr2.IsOwned()); + + CFX_MaybeOwned ptr3(std::move(ptr1)); + CFX_MaybeOwned ptr4(std::move(ptr2)); + EXPECT_FALSE(ptr1.IsOwned()); + EXPECT_FALSE(ptr2.IsOwned()); + EXPECT_FALSE(ptr3.IsOwned()); + EXPECT_TRUE(ptr4.IsOwned()); + EXPECT_EQ(0, delete_count); + EXPECT_EQ(nullptr, ptr1.Get()); + EXPECT_EQ(nullptr, ptr2.Get()); + EXPECT_EQ(100, ptr3->GetID()); + EXPECT_EQ(200, ptr4->GetID()); + + CFX_MaybeOwned ptr5; + CFX_MaybeOwned ptr6; + ptr5 = std::move(ptr3); + ptr6 = std::move(ptr4); + EXPECT_FALSE(ptr3.IsOwned()); + EXPECT_FALSE(ptr4.IsOwned()); + EXPECT_FALSE(ptr5.IsOwned()); + EXPECT_TRUE(ptr6.IsOwned()); + EXPECT_EQ(0, delete_count); + EXPECT_EQ(nullptr, ptr3.Get()); + EXPECT_EQ(nullptr, ptr4.Get()); + EXPECT_EQ(100, ptr5->GetID()); + EXPECT_EQ(200, ptr6->GetID()); + } + EXPECT_EQ(1, delete_count); +} -- cgit v1.2.3