From 931d087b991a986cc8bfa72131fe8eda0d987fc4 Mon Sep 17 00:00:00 2001 From: tsepez Date: Fri, 4 Nov 2016 16:00:03 -0700 Subject: Pass object to ReplaceIndirectObjectIfHigherGeneration() by unique_ptr. Review-Url: https://codereview.chromium.org/2477003002 --- core/fpdfapi/parser/cfdf_document.cpp | 5 +++-- core/fpdfapi/parser/cpdf_array_unittest.cpp | 10 ++++++---- core/fpdfapi/parser/cpdf_data_avail.cpp | 10 ++++------ core/fpdfapi/parser/cpdf_indirect_object_holder.cpp | 9 ++++----- core/fpdfapi/parser/cpdf_indirect_object_holder.h | 5 +++-- core/fpdfapi/parser/cpdf_number.h | 3 +-- core/fpdfapi/parser/cpdf_object_unittest.cpp | 5 ++--- core/fpdfapi/parser/cpdf_parser.cpp | 3 +-- 8 files changed, 24 insertions(+), 26 deletions(-) diff --git a/core/fpdfapi/parser/cfdf_document.cpp b/core/fpdfapi/parser/cfdf_document.cpp index e66bfeb12b..a0f3e1490d 100644 --- a/core/fpdfapi/parser/cfdf_document.cpp +++ b/core/fpdfapi/parser/cfdf_document.cpp @@ -69,11 +69,12 @@ void CFDF_Document::ParseStream(IFX_SeekableReadStream* pFile, bool bOwnFile) { if (word != "obj") break; - CPDF_Object* pObj = parser.GetObject(this, objnum, 0, true); + auto pObj = pdfium::WrapUnique( + parser.GetObject(this, objnum, 0, true)); if (!pObj) break; - ReplaceIndirectObjectIfHigherGeneration(objnum, pObj); + ReplaceIndirectObjectIfHigherGeneration(objnum, std::move(pObj)); word = parser.GetNextWord(nullptr); if (word != "endobj") break; diff --git a/core/fpdfapi/parser/cpdf_array_unittest.cpp b/core/fpdfapi/parser/cpdf_array_unittest.cpp index 800afb0f9a..b1a4605666 100644 --- a/core/fpdfapi/parser/cpdf_array_unittest.cpp +++ b/core/fpdfapi/parser/cpdf_array_unittest.cpp @@ -3,12 +3,13 @@ // found in the LICENSE file. #include "core/fpdfapi/parser/cpdf_array.h" -#include "core/fpdfapi/parser/cpdf_number.h" -#include "core/fpdfapi/parser/cpdf_reference.h" #include +#include "core/fpdfapi/parser/cpdf_number.h" +#include "core/fpdfapi/parser/cpdf_reference.h" #include "testing/gtest/include/gtest/gtest.h" +#include "third_party/base/ptr_util.h" TEST(cpdf_array, RemoveAt) { { @@ -107,10 +108,11 @@ TEST(cpdf_array, Clone) { for (size_t i = 0; i < kNumOfRows; ++i) { CPDF_Array* arr_elem = new CPDF_Array; for (size_t j = 0; j < kNumOfRowElems; ++j) { - CPDF_Number* obj = new CPDF_Number(elems[i][j]); + std::unique_ptr obj(new CPDF_Number(elems[i][j])); // Starts object number from 1. int obj_num = i * kNumOfRowElems + j + 1; - obj_holder->ReplaceIndirectObjectIfHigherGeneration(obj_num, obj); + obj_holder->ReplaceIndirectObjectIfHigherGeneration(obj_num, + std::move(obj)); arr_elem->InsertAt(j, new CPDF_Reference(obj_holder.get(), obj_num)); } arr->InsertAt(i, arr_elem); diff --git a/core/fpdfapi/parser/cpdf_data_avail.cpp b/core/fpdfapi/parser/cpdf_data_avail.cpp index eadbf1e828..c4ed95e17f 100644 --- a/core/fpdfapi/parser/cpdf_data_avail.cpp +++ b/core/fpdfapi/parser/cpdf_data_avail.cpp @@ -1763,13 +1763,11 @@ CPDF_Dictionary* CPDF_DataAvail::GetPage(int index) { // We should say to the document, which object is the page. m_pDocument->SetPageObjNum(index, dwObjNum); // Page object already can be parsed in document. - CPDF_Object* pPageDict = m_pDocument->GetIndirectObject(dwObjNum); - if (!pPageDict) { + if (!m_pDocument->GetIndirectObject(dwObjNum)) { m_syntaxParser.InitParser(m_pFileRead, (uint32_t)szPageStartPos); - pPageDict = ParseIndirectObjectAt(0, dwObjNum, m_pDocument); - if (pPageDict) { - m_pDocument->ReplaceIndirectObjectIfHigherGeneration(dwObjNum, pPageDict); - } + m_pDocument->ReplaceIndirectObjectIfHigherGeneration( + dwObjNum, pdfium::WrapUnique( + ParseIndirectObjectAt(0, dwObjNum, m_pDocument))); } return m_pDocument->GetPage(index); } diff --git a/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp b/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp index 720fe18606..b2cb08c08a 100644 --- a/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp +++ b/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp @@ -53,18 +53,17 @@ uint32_t CPDF_IndirectObjectHolder::AddIndirectObject(CPDF_Object* pObj) { bool CPDF_IndirectObjectHolder::ReplaceIndirectObjectIfHigherGeneration( uint32_t objnum, - CPDF_Object* pObj) { + std::unique_ptr pObj) { ASSERT(objnum); if (!pObj) return false; CPDF_Object* pOldObj = GetIndirectObject(objnum); - if (pOldObj && pObj->GetGenNum() <= pOldObj->GetGenNum()) { - delete pObj; + if (pOldObj && pObj->GetGenNum() <= pOldObj->GetGenNum()) return false; - } + pObj->m_ObjNum = objnum; - m_IndirectObjs[objnum].reset(pObj); + m_IndirectObjs[objnum] = std::move(pObj); m_LastObjNum = std::max(m_LastObjNum, objnum); return true; } diff --git a/core/fpdfapi/parser/cpdf_indirect_object_holder.h b/core/fpdfapi/parser/cpdf_indirect_object_holder.h index 83a31535c4..fcd77215eb 100644 --- a/core/fpdfapi/parser/cpdf_indirect_object_holder.h +++ b/core/fpdfapi/parser/cpdf_indirect_object_holder.h @@ -28,8 +28,9 @@ class CPDF_IndirectObjectHolder { // Take ownership of |pObj|. uint32_t AddIndirectObject(CPDF_Object* pObj); - bool ReplaceIndirectObjectIfHigherGeneration(uint32_t objnum, - CPDF_Object* pObj); + bool ReplaceIndirectObjectIfHigherGeneration( + uint32_t objnum, + std::unique_ptr pObj); uint32_t GetLastObjNum() const { return m_LastObjNum; } void SetLastObjNum(uint32_t objnum) { m_LastObjNum = objnum; } diff --git a/core/fpdfapi/parser/cpdf_number.h b/core/fpdfapi/parser/cpdf_number.h index a5efc1423e..717c2b7887 100644 --- a/core/fpdfapi/parser/cpdf_number.h +++ b/core/fpdfapi/parser/cpdf_number.h @@ -17,6 +17,7 @@ class CPDF_Number : public CPDF_Object { explicit CPDF_Number(int value); explicit CPDF_Number(FX_FLOAT value); explicit CPDF_Number(const CFX_ByteStringC& str); + ~CPDF_Number() override; // CPDF_Object. Type GetType() const override; @@ -32,8 +33,6 @@ class CPDF_Number : public CPDF_Object { bool IsInteger() const { return m_bInteger; } protected: - ~CPDF_Number() override; - bool m_bInteger; union { int m_Integer; diff --git a/core/fpdfapi/parser/cpdf_object_unittest.cpp b/core/fpdfapi/parser/cpdf_object_unittest.cpp index 64dc8c63ec..1bcf6164be 100644 --- a/core/fpdfapi/parser/cpdf_object_unittest.cpp +++ b/core/fpdfapi/parser/cpdf_object_unittest.cpp @@ -722,9 +722,8 @@ TEST(PDFArrayTest, AddReferenceAndGetObjectAt) { std::unique_ptr arr1(new CPDF_Array); // Create two arrays of references by different AddReference() APIs. for (size_t i = 0; i < FX_ArraySize(indirect_objs); ++i) { - // All the indirect objects inserted will be owned by holder. - holder->ReplaceIndirectObjectIfHigherGeneration(obj_nums[i], - indirect_objs[i]); + holder->ReplaceIndirectObjectIfHigherGeneration( + obj_nums[i], pdfium::WrapUnique(indirect_objs[i])); arr->AddReference(holder.get(), obj_nums[i]); arr1->AddReference(holder.get(), indirect_objs[i]->GetObjNum()); } diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp index 91f157c269..c8c07bd0b7 100644 --- a/core/fpdfapi/parser/cpdf_parser.cpp +++ b/core/fpdfapi/parser/cpdf_parser.cpp @@ -956,9 +956,8 @@ bool CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, bool bMainXRef) { CPDF_Dictionary* pRootDict = m_pDocument->GetRoot(); if (pRootDict && pRootDict->GetObjNum() == objnum) return false; - // Takes ownership of object (std::move someday). if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration( - objnum, pObject.release())) { + objnum, std::move(pObject))) { return false; } } -- cgit v1.2.3