Simplify CPDF_Array::RemoveAt(index, size).

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 <thestig@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>

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<T>(m_pPool, std::forward<Args>(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<CPDF_Array>();
     for (size_t i = 0; i < FX_ArraySize(elems); ++i)
       arr->AddNew<CPDF_Number>(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<CPDF_Array>();
     for (size_t i = 0; i < FX_ArraySize(elems); ++i)
       arr->AddNew<CPDF_Number>(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<CPDF_Array>();
+  EXPECT_EQ(0U, arr->GetCount());
+  for (size_t i = 0; i < FX_ArraySize(elems); ++i)
+    arr->AddNew<CPDF_Number>(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<CPDF_Array>();
     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<CPDF_Number>(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<CPDF_Array>();
+    for (size_t i = 0; i < FX_ArraySize(elems); ++i)
+      arr->AddNew<CPDF_Number>(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<CPDF_Array>();
     for (size_t i = 0; i < FX_ArraySize(elems); ++i)
       arr->InsertNewAt<CPDF_Number>(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<CPDF_Number>(3, 33);
     arr->InsertNewAt<CPDF_Number>(6, 55);
     arr->InsertNewAt<CPDF_Number>(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<CPDF_Array>();
     for (size_t i = 0; i < FX_ArraySize(elems); ++i)
       arr->InsertNewAt<CPDF_Number>(i, elems[i]);
     arr->InsertNewAt<CPDF_Number>(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<CPDF_Array>();
     for (size_t i = 0; i < FX_ArraySize(elems); ++i)
       arr->InsertNewAt<CPDF_Number>(i, elems[i]);
     std::unique_ptr<CPDF_Array> 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<CPDF_Array>();
     // 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<CPDF_Array> 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<CPDF_Array> 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<CPDF_Array>(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<CPDF_Array>("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<CPDF_Array>("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<CPDF_Array>("Rect");
+      pRectArray = pAnnotDict->SetNewFor<CPDF_Array>("Rect");
 
     pRectArray->AddNew<CPDF_Number>(rect.left);
     pRectArray->AddNew<CPDF_Number>(rect.bottom);