From 59c1ac070d1c8ca70e96805b697c5635ccd70cdf Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Fri, 9 Jun 2017 11:04:41 -0700 Subject: Simplify CPDF_Array::RemoveAt(index, size). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of one general RemoveAt() method, split it into: - RemoveAt(index) - Truncate(nNewSize) - Clear() Update callers, which are now easier to understand. Update existing unit tests and add new tests. Change-Id: I38fe40146ce8f2479677b2caadd20a1756678768 Reviewed-on: https://pdfium-review.googlesource.com/6417 Commit-Queue: Lei Zhang Reviewed-by: Nicolás Peña --- core/fpdfapi/parser/cpdf_array.cpp | 16 ++++-- core/fpdfapi/parser/cpdf_array.h | 4 +- core/fpdfapi/parser/cpdf_array_unittest.cpp | 84 ++++++++++++++++++++--------- core/fpdfapi/parser/fpdf_parser_decode.cpp | 2 +- fpdfsdk/fpdfannot.cpp | 6 +-- fpdfsdk/fpdfeditpage.cpp | 7 +-- 6 files changed, 81 insertions(+), 38 deletions(-) diff --git a/core/fpdfapi/parser/cpdf_array.cpp b/core/fpdfapi/parser/cpdf_array.cpp index 4caad10631..ea4ca7eaeb 100644 --- a/core/fpdfapi/parser/cpdf_array.cpp +++ b/core/fpdfapi/parser/cpdf_array.cpp @@ -136,14 +136,20 @@ CPDF_Array* CPDF_Array::GetArrayAt(size_t i) const { return ToArray(GetDirectObjectAt(i)); } -void CPDF_Array::RemoveAt(size_t i, size_t nCount) { - if (i >= m_Objects.size()) - return; +void CPDF_Array::RemoveAt(size_t i) { + if (i < m_Objects.size()) + m_Objects.erase(m_Objects.begin() + i); +} + +void CPDF_Array::Clear() { + m_Objects.clear(); +} - if (nCount <= 0 || nCount > m_Objects.size() - i) +void CPDF_Array::Truncate(size_t nNewSize) { + if (nNewSize >= m_Objects.size()) return; - m_Objects.erase(m_Objects.begin() + i, m_Objects.begin() + i + nCount); + m_Objects.resize(nNewSize); } void CPDF_Array::ConvertToIndirectObjectAt(size_t i, diff --git a/core/fpdfapi/parser/cpdf_array.h b/core/fpdfapi/parser/cpdf_array.h index aff65d1454..bb17c0a427 100644 --- a/core/fpdfapi/parser/cpdf_array.h +++ b/core/fpdfapi/parser/cpdf_array.h @@ -99,7 +99,9 @@ class CPDF_Array : public CPDF_Object { index, pdfium::MakeUnique(m_pPool, std::forward(args)...))); } - void RemoveAt(size_t index, size_t nCount = 1); + void RemoveAt(size_t index); + void Clear(); + void Truncate(size_t nNewSize); void ConvertToIndirectObjectAt(size_t index, CPDF_IndirectObjectHolder* pDoc); const_iterator begin() const { return m_Objects.begin(); } diff --git a/core/fpdfapi/parser/cpdf_array_unittest.cpp b/core/fpdfapi/parser/cpdf_array_unittest.cpp index 1e92b32716..006e5fad33 100644 --- a/core/fpdfapi/parser/cpdf_array_unittest.cpp +++ b/core/fpdfapi/parser/cpdf_array_unittest.cpp @@ -14,52 +14,85 @@ TEST(cpdf_array, RemoveAt) { { - int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + const int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; auto arr = pdfium::MakeUnique(); for (size_t i = 0; i < FX_ArraySize(elems); ++i) arr->AddNew(elems[i]); - arr->RemoveAt(3, 3); - int expected[] = {1, 2, 3, 7, 8, 9, 10}; - EXPECT_EQ(FX_ArraySize(expected), arr->GetCount()); + for (size_t i = 0; i < 3; ++i) + arr->RemoveAt(3); + const int expected[] = {1, 2, 3, 7, 8, 9, 10}; + ASSERT_EQ(FX_ArraySize(expected), arr->GetCount()); for (size_t i = 0; i < FX_ArraySize(expected); ++i) EXPECT_EQ(expected[i], arr->GetIntegerAt(i)); - arr->RemoveAt(4, 2); - int expected2[] = {1, 2, 3, 7, 10}; - EXPECT_EQ(FX_ArraySize(expected2), arr->GetCount()); + arr->RemoveAt(4); + arr->RemoveAt(4); + const int expected2[] = {1, 2, 3, 7, 10}; + ASSERT_EQ(FX_ArraySize(expected2), arr->GetCount()); for (size_t i = 0; i < FX_ArraySize(expected2); ++i) EXPECT_EQ(expected2[i], arr->GetIntegerAt(i)); } { - // When the range is out of bound, RemoveAt has no effect. - int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + // When the range is out of bound, RemoveAt() has no effect. + const int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; auto arr = pdfium::MakeUnique(); for (size_t i = 0; i < FX_ArraySize(elems); ++i) arr->AddNew(elems[i]); - arr->RemoveAt(8, 5); + arr->RemoveAt(11); EXPECT_EQ(FX_ArraySize(elems), arr->GetCount()); + } +} + +TEST(cpdf_array, Clear) { + const int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + auto arr = pdfium::MakeUnique(); + EXPECT_EQ(0U, arr->GetCount()); + for (size_t i = 0; i < FX_ArraySize(elems); ++i) + arr->AddNew(elems[i]); + EXPECT_EQ(FX_ArraySize(elems), arr->GetCount()); + arr->Clear(); + EXPECT_EQ(0U, arr->GetCount()); +} + +TEST(cpdf_array, Truncate) { + { + const int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + auto arr = pdfium::MakeUnique(); for (size_t i = 0; i < FX_ArraySize(elems); ++i) - EXPECT_EQ(elems[i], arr->GetIntegerAt(i)); - arr->RemoveAt(0, 12); - EXPECT_EQ(FX_ArraySize(elems), arr->GetCount()); - arr->RemoveAt(11, 1); + arr->AddNew(elems[i]); + arr->Truncate(4); + const int expected[] = {1, 2, 3, 4}; + ASSERT_EQ(FX_ArraySize(expected), arr->GetCount()); + for (size_t i = 0; i < FX_ArraySize(expected); ++i) + EXPECT_EQ(expected[i], arr->GetIntegerAt(i)); + arr->Truncate(0); + EXPECT_EQ(0U, arr->GetCount()); + } + { + // When the range is out of bound, Truncate() has no effect. + // It does not try to grow the array. + const int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + auto arr = pdfium::MakeUnique(); + for (size_t i = 0; i < FX_ArraySize(elems); ++i) + arr->AddNew(elems[i]); + arr->Truncate(11); EXPECT_EQ(FX_ArraySize(elems), arr->GetCount()); } } TEST(cpdf_array, InsertAt) { { - int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + const int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; auto arr = pdfium::MakeUnique(); for (size_t i = 0; i < FX_ArraySize(elems); ++i) arr->InsertNewAt(i, elems[i]); - EXPECT_EQ(FX_ArraySize(elems), arr->GetCount()); + ASSERT_EQ(FX_ArraySize(elems), arr->GetCount()); for (size_t i = 0; i < FX_ArraySize(elems); ++i) EXPECT_EQ(elems[i], arr->GetIntegerAt(i)); arr->InsertNewAt(3, 33); arr->InsertNewAt(6, 55); arr->InsertNewAt(12, 12); - int expected[] = {1, 2, 3, 33, 4, 5, 55, 6, 7, 8, 9, 10, 12}; - EXPECT_EQ(FX_ArraySize(expected), arr->GetCount()); + const int expected[] = {1, 2, 3, 33, 4, 5, 55, 6, 7, 8, 9, 10, 12}; + ASSERT_EQ(FX_ArraySize(expected), arr->GetCount()); for (size_t i = 0; i < FX_ArraySize(expected); ++i) EXPECT_EQ(expected[i], arr->GetIntegerAt(i)); } @@ -67,12 +100,12 @@ TEST(cpdf_array, InsertAt) { // When the position to insert is beyond the upper bound, // an element is inserted at that position while other unfilled // positions have nullptr. - int elems[] = {1, 2}; + const int elems[] = {1, 2}; auto arr = pdfium::MakeUnique(); for (size_t i = 0; i < FX_ArraySize(elems); ++i) arr->InsertNewAt(i, elems[i]); arr->InsertNewAt(10, 10); - EXPECT_EQ(11u, arr->GetCount()); + ASSERT_EQ(11u, arr->GetCount()); for (size_t i = 0; i < FX_ArraySize(elems); ++i) EXPECT_EQ(elems[i], arr->GetIntegerAt(i)); for (size_t i = FX_ArraySize(elems); i < 10; ++i) @@ -84,12 +117,12 @@ TEST(cpdf_array, InsertAt) { TEST(cpdf_array, Clone) { { // Basic case. - int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; + const int elems[] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10}; auto arr = pdfium::MakeUnique(); for (size_t i = 0; i < FX_ArraySize(elems); ++i) arr->InsertNewAt(i, elems[i]); std::unique_ptr arr2 = ToArray(arr->Clone()); - EXPECT_EQ(arr->GetCount(), arr2->GetCount()); + ASSERT_EQ(arr->GetCount(), arr2->GetCount()); for (size_t i = 0; i < FX_ArraySize(elems); ++i) { // Clone() always create new objects. EXPECT_NE(arr->GetObjectAt(i), arr2->GetObjectAt(i)); @@ -100,7 +133,7 @@ TEST(cpdf_array, Clone) { // Clone() with and without dereferencing reference objects. static const size_t kNumOfRows = 3; static const size_t kNumOfRowElems = 5; - int elems[kNumOfRows][kNumOfRowElems] = { + const int elems[kNumOfRows][kNumOfRowElems] = { {1, 2, 3, 4, 5}, {10, 9, 8, 7, 6}, {11, 12, 13, 14, 15}}; auto arr = pdfium::MakeUnique(); // Indirect references to indirect objects. @@ -121,10 +154,10 @@ TEST(cpdf_array, Clone) { // Not dereferencing reference objects means just creating new references // instead of new copies of direct objects. std::unique_ptr arr1 = ToArray(arr->Clone()); - EXPECT_EQ(arr->GetCount(), arr1->GetCount()); + ASSERT_EQ(arr->GetCount(), arr1->GetCount()); // Dereferencing reference objects creates new copies of direct objects. std::unique_ptr arr2 = ToArray(arr->CloneDirectObject()); - EXPECT_EQ(arr->GetCount(), arr2->GetCount()); + ASSERT_EQ(arr->GetCount(), arr2->GetCount()); for (size_t i = 0; i < kNumOfRows; ++i) { CPDF_Array* arr_elem = arr->GetObjectAt(i)->AsArray(); CPDF_Array* arr1_elem = arr1->GetObjectAt(i)->AsArray(); @@ -173,4 +206,5 @@ TEST(cpdf_array, Iterator) { size_t index = 0; for (const auto& it : *arr) EXPECT_EQ(elems[index++], it->AsNumber()->GetInteger()); + EXPECT_EQ(FX_ArraySize(elems), index); } diff --git a/core/fpdfapi/parser/fpdf_parser_decode.cpp b/core/fpdfapi/parser/fpdf_parser_decode.cpp index 561b6627bc..3d7b9ffd59 100644 --- a/core/fpdfapi/parser/fpdf_parser_decode.cpp +++ b/core/fpdfapi/parser/fpdf_parser_decode.cpp @@ -400,7 +400,7 @@ bool PDF_DataDecode(const uint8_t* src_buf, dest_buf = last_buf; dest_size = last_size; if (CPDF_Array* pDecoders = pDecoder->AsArray()) - pDecoders->RemoveAt(i + 1, pDecoders->GetCount() - i - 1); + pDecoders->Truncate(i + 1); return true; } if (last_buf != src_buf) { diff --git a/fpdfsdk/fpdfannot.cpp b/fpdfsdk/fpdfannot.cpp index 941a5fdd80..0c2152a7aa 100644 --- a/fpdfsdk/fpdfannot.cpp +++ b/fpdfsdk/fpdfannot.cpp @@ -184,7 +184,7 @@ DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_SetColor(FPDF_ANNOTATION annot, CFX_ByteString key = type == FPDFANNOT_COLORTYPE_InteriorColor ? "IC" : "C"; CPDF_Array* pColor = pAnnotDict->GetArrayFor(key); if (pColor) - pColor->RemoveAt(0, pColor->GetCount()); + pColor->Clear(); else pColor = pAnnotDict->SetNewFor(key); @@ -270,7 +270,7 @@ FPDFAnnot_SetAttachmentPoints(FPDF_ANNOTATION annot, CPDF_Dictionary* pAnnotDict = CPDFDictionaryFromFPDFAnnotation(annot); CPDF_Array* pQuadPoints = pAnnotDict->GetArrayFor("QuadPoints"); if (pQuadPoints) - pQuadPoints->RemoveAt(0, pQuadPoints->GetCount()); + pQuadPoints->Clear(); else pQuadPoints = pAnnotDict->SetNewFor("QuadPoints"); @@ -315,7 +315,7 @@ DLLEXPORT FPDF_BOOL STDCALL FPDFAnnot_SetRect(FPDF_ANNOTATION annot, CPDF_Array* pRect = pAnnotDict->GetArrayFor("Rect"); if (pRect) - pRect->RemoveAt(0, pRect->GetCount()); + pRect->Clear(); else pRect = pAnnotDict->SetNewFor("Rect"); diff --git a/fpdfsdk/fpdfeditpage.cpp b/fpdfsdk/fpdfeditpage.cpp index 22b2261c6f..699e030c49 100644 --- a/fpdfsdk/fpdfeditpage.cpp +++ b/fpdfsdk/fpdfeditpage.cpp @@ -297,11 +297,12 @@ DLLEXPORT void STDCALL FPDFPage_TransformAnnots(FPDF_PAGE page, (float)f); matrix.TransformRect(rect); - CPDF_Array* pRectArray = pAnnot->GetAnnotDict()->GetArrayFor("Rect"); + CPDF_Dictionary* pAnnotDict = pAnnot->GetAnnotDict(); + CPDF_Array* pRectArray = pAnnotDict->GetArrayFor("Rect"); if (pRectArray) - pRectArray->RemoveAt(0, pRectArray->GetCount()); + pRectArray->Clear(); else - pRectArray = pAnnot->GetAnnotDict()->SetNewFor("Rect"); + pRectArray = pAnnotDict->SetNewFor("Rect"); pRectArray->AddNew(rect.left); pRectArray->AddNew(rect.bottom); -- cgit v1.2.3