Refactor QPDF_Array::eraseItem and rename to erase

6 files changed, 28 insertions, 29 deletions

diff --git a/libqpdf/QPDFObjectHandle.cc b/libqpdf/QPDFObjectHandle.cc
index fa8b9136..e113089a 100644
--- a/libqpdf/QPDFObjectHandle.cc
+++ b/libqpdf/QPDFObjectHandle.cc
@@ -972,17 +972,14 @@ QPDFObjectHandle::appendItemAndGetNew(QPDFObjectHandle const& item)
 void
 QPDFObjectHandle::eraseItem(int at)
 {
-    auto array = asArray();
-    if (array && at < array->size() && at >= 0) {
-        array->eraseItem(at);
-    } else {
-        if (array) {
+    if (auto array = asArray()) {
+        if (!array->erase(at)) {
             objectWarning("ignoring attempt to erase out of bounds array item");
             QTC::TC("qpdf", "QPDFObjectHandle erase array bounds");
-        } else {
-            typeWarning("array", "ignoring attempt to erase item");
-            QTC::TC("qpdf", "QPDFObjectHandle array ignoring erase item");
         }
+    } else {
+        typeWarning("array", "ignoring attempt to erase item");
+        QTC::TC("qpdf", "QPDFObjectHandle array ignoring erase item");
     }
 }
 
diff --git a/libqpdf/QPDF_Array.cc b/libqpdf/QPDF_Array.cc
index 7633266e..e4e1668e 100644
--- a/libqpdf/QPDF_Array.cc
+++ b/libqpdf/QPDF_Array.cc
@@ -267,17 +267,16 @@ QPDF_Array::push_back(QPDFObjectHandle const& item)
     }
 }
 
-void
-QPDF_Array::eraseItem(int at)
+bool
+QPDF_Array::erase(int at)
 {
+    if (at < 0 || at >= size()) {
+        return false;
+    }
     if (sparse) {
         sp_elements.erase(at);
     } else {
-        size_t idx = QIntC::to_size(at);
-        if (idx >= elements.size()) {
-            throw std::logic_error("bounds error erasing item from OHArray");
-        }
-        int n = int(idx);
-        elements.erase(elements.cbegin() + n);
+        elements.erase(elements.cbegin() + at);
     }
+    return true;
 }
diff --git a/libqpdf/SparseOHArray.cc b/libqpdf/SparseOHArray.cc
index 8f6c02d7..773d7309 100644
--- a/libqpdf/SparseOHArray.cc
+++ b/libqpdf/SparseOHArray.cc
@@ -44,21 +44,22 @@ SparseOHArray::setAt(int idx, QPDFObjectHandle oh)
 }
 
 void
-SparseOHArray::erase(int idx)
+SparseOHArray::erase(int at)
 {
-    if (idx >= this->n_elements) {
-        throw std::logic_error("bounds error erasing item from SparseOHArray");
-    }
-    decltype(this->elements) dest;
-    for (auto const& iter: this->elements) {
-        if (iter.first < idx) {
-            dest.insert(iter);
-        } else if (iter.first > idx) {
-            dest[iter.first - 1] = iter.second;
+    auto end = elements.end();
+    if (auto iter = elements.lower_bound(at); iter != end) {
+        if (iter->first == at) {
+            iter++;
+            elements.erase(at);
+        }
+
+        while (iter != end) {
+            auto nh = elements.extract(iter++);
+            --nh.key();
+            elements.insert(std::move(nh));
         }
     }
-    this->elements = dest;
-    --this->n_elements;
+    --n_elements;
 }
 
 void
diff --git a/libqpdf/qpdf/QPDF_Array.hh b/libqpdf/qpdf/QPDF_Array.hh
index 8768c9ad..edda54ec 100644
--- a/libqpdf/qpdf/QPDF_Array.hh
+++ b/libqpdf/qpdf/QPDF_Array.hh
@@ -35,7 +35,7 @@ class QPDF_Array: public QPDFValue
     void setFromVector(std::vector<std::shared_ptr<QPDFObject>>&& items);
     bool insert(int at, QPDFObjectHandle const& item);
     void push_back(QPDFObjectHandle const& item);
-    void eraseItem(int at);
+    bool erase(int at);
 
   private:
     QPDF_Array(std::vector<QPDFObjectHandle> const& items);
diff --git a/qpdf/qtest/qpdf/test88.out b/qpdf/qtest/qpdf/test88.out
index 327212b6..9d5cf504 100644
--- a/qpdf/qtest/qpdf/test88.out
+++ b/qpdf/qtest/qpdf/test88.out
@@ -1,2 +1,3 @@
 WARNING: test array: ignoring attempt to erase out of bounds array item
+WARNING: minimal.pdf, object 1 0 at offset 19: operation for array attempted on object of type dictionary: ignoring attempt to erase item
 test 88 done
diff --git a/qpdf/test_driver.cc b/qpdf/test_driver.cc
index 8c54ad01..82361ba0 100644
--- a/qpdf/test_driver.cc
+++ b/qpdf/test_driver.cc
@@ -3283,6 +3283,7 @@ test_88(QPDF& pdf, char const* arg2)
     auto arr2 = pdf.getRoot().replaceKeyAndGetNew("/QTest", "[1 2]"_qpdf);
     arr2.setObjectDescription(&pdf, "test array");
     assert(arr2.eraseItemAndGetOld(50).isNull());
+    assert(pdf.getRoot().eraseItemAndGetOld(0).isNull());
 }
 
 static void