Merge pull request #863 from m-holger/array

Refactor QPDF_Array

include/qpdf/QPDFObjectHandle.hh

@@ -1496,11 +1496,10 @@ class QPDFObjectHandle
     {
         friend class QPDF_Dictionary;
         friend class QPDF_Stream;
-        friend class SparseOHArray;
 
       private:
         static void
-        disconnect(QPDFObjectHandle& o)
+        disconnect(QPDFObjectHandle o)
         {
             o.disconnect();
         }
@@ -1577,6 +1576,11 @@ class QPDFObjectHandle
     {
         return obj;
     }
+    std::shared_ptr<QPDFObject>
+    getObj() const
+    {
+        return obj;
+    }
     QPDFObject*
     getObjectPtr()
     {

libqpdf/CMakeLists.txt

@@ -115,7 +115,6 @@ set(libqpdf_SOURCES
   ResourceFinder.cc
   SecureRandomDataProvider.cc
   SF_FlateLzwDecode.cc
-  SparseOHArray.cc
   qpdf-c.cc
   qpdfjob-c.cc
   qpdflogger-c.cc)

libqpdf/QPDFObjectHandle.cc

@@ -23,7 +23,6 @@
 #include <qpdf/QPDF_Stream.hh>
 #include <qpdf/QPDF_String.hh>
 #include <qpdf/QPDF_Unresolved.hh>
-#include <qpdf/SparseOHArray.hh>
 
 #include <qpdf/QIntC.hh>
 #include <qpdf/QTC.hh>
@@ -789,9 +788,8 @@ QPDFObjectHandle::aitems()
 int
 QPDFObjectHandle::getArrayNItems()
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array) {
+        return array->size();
-        return array->getNItems();
     } else {
         typeWarning("array", "treating as empty");
         QTC::TC("qpdf", "QPDFObjectHandle array treating as empty");
@@ -802,104 +800,101 @@ QPDFObjectHandle::getArrayNItems()
 QPDFObjectHandle
 QPDFObjectHandle::getArrayItem(int n)
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array && (n < array->getNItems()) && (n >= 0)) {
+        if (auto result = array->at(n); result.obj != nullptr) {
-        return array->getItem(n);
+            return result;
-    } else {
+        } else {
-        if (array) {
             objectWarning("returning null for out of bounds array access");
             QTC::TC("qpdf", "QPDFObjectHandle array bounds");
-        } else {
-            typeWarning("array", "returning null");
-            QTC::TC("qpdf", "QPDFObjectHandle array null for non-array");
         }
-        static auto constexpr msg =
+    } else {
-            " -> null returned from invalid array access"sv;
+        typeWarning("array", "returning null");
-        return QPDF_Null::create(obj, msg, "");
+        QTC::TC("qpdf", "QPDFObjectHandle array null for non-array");
     }
+    static auto constexpr msg = " -> null returned from invalid array access"sv;
+    return QPDF_Null::create(obj, msg, "");
 }
 
 bool
 QPDFObjectHandle::isRectangle()
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if ((array == nullptr) || (array->getNItems() != 4)) {
+        for (int i = 0; i < 4; ++i) {
-        return false;
+            if (auto item = array->at(i); !(item.obj && item.isNumber())) {
-    }
+                return false;
-    for (int i = 0; i < 4; ++i) {
+            }
-        if (!array->getItem(i).isNumber()) {
-            return false;
         }
+        return array->size() == 4;
     }
-    return true;
+    return false;
 }
 
 bool
 QPDFObjectHandle::isMatrix()
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if ((array == nullptr) || (array->getNItems() != 6)) {
+        for (int i = 0; i < 6; ++i) {
-        return false;
+            if (auto item = array->at(i); !(item.obj && item.isNumber())) {
-    }
+                return false;
-    for (int i = 0; i < 6; ++i) {
+            }
-        if (!array->getItem(i).isNumber()) {
-            return false;
         }
+        return array->size() == 6;
     }
-    return true;
+    return false;
 }
 
 QPDFObjectHandle::Rectangle
 QPDFObjectHandle::getArrayAsRectangle()
 {
-    Rectangle result;
+    if (auto array = asArray()) {
-    if (isRectangle()) {
+        if (array->size() != 4) {
-        auto array = asArray();
+            return {};
-        // Rectangle coordinates are always supposed to be llx, lly,
+        }
-        // urx, ury, but files have been found in the wild where
+        double items[4];
-        // llx > urx or lly > ury.
+        for (int i = 0; i < 4; ++i) {
-        double i0 = array->getItem(0).getNumericValue();
+            if (!array->at(i).getValueAsNumber(items[i])) {
-        double i1 = array->getItem(1).getNumericValue();
+                return {};
-        double i2 = array->getItem(2).getNumericValue();
+            }
-        double i3 = array->getItem(3).getNumericValue();
+        }
-        result = Rectangle(
+        return Rectangle(
-            std::min(i0, i2),
+            std::min(items[0], items[2]),
-            std::min(i1, i3),
+            std::min(items[1], items[3]),
-            std::max(i0, i2),
+            std::max(items[0], items[2]),
-            std::max(i1, i3));
+            std::max(items[1], items[3]));
     }
-    return result;
+    return {};
 }
 
 QPDFObjectHandle::Matrix
 QPDFObjectHandle::getArrayAsMatrix()
 {
-    Matrix result;
+    if (auto array = asArray()) {
-    if (isMatrix()) {
+        if (array->size() != 6) {
-        auto array = asArray();
+            return {};
-        result = Matrix(
+        }
-            array->getItem(0).getNumericValue(),
+        double items[6];
-            array->getItem(1).getNumericValue(),
+        for (int i = 0; i < 6; ++i) {
-            array->getItem(2).getNumericValue(),
+            if (!array->at(i).getValueAsNumber(items[i])) {
-            array->getItem(3).getNumericValue(),
+                return {};
-            array->getItem(4).getNumericValue(),
+            }
-            array->getItem(5).getNumericValue());
+        }
+        return Matrix(
+            items[0], items[1], items[2], items[3], items[4], items[5]);
     }
-    return result;
+    return {};
 }
 
 std::vector<QPDFObjectHandle>
 QPDFObjectHandle::getArrayAsVector()
 {
-    std::vector<QPDFObjectHandle> result;
     auto array = asArray();
     if (array) {
-        array->getAsVector(result);
+        return array->getAsVector();
     } else {
         typeWarning("array", "treating as empty");
         QTC::TC("qpdf", "QPDFObjectHandle array treating as empty vector");
     }
-    return result;
+    return {};
 }
 
 // Array mutators
@@ -907,24 +902,20 @@ QPDFObjectHandle::getArrayAsVector()
 void
 QPDFObjectHandle::setArrayItem(int n, QPDFObjectHandle const& item)
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array) {
+        if (!array->setAt(n, item)) {
-        checkOwnership(item);
+            objectWarning("ignoring attempt to set out of bounds array item");
-        array->setItem(n, item);
+            QTC::TC("qpdf", "QPDFObjectHandle set array bounds");
+        }
     } else {
         typeWarning("array", "ignoring attempt to set item");
         QTC::TC("qpdf", "QPDFObjectHandle array ignoring set item");
     }
 }
-
 void
 QPDFObjectHandle::setArrayFromVector(std::vector<QPDFObjectHandle> const& items)
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array) {
-        for (auto const& item: items) {
-            checkOwnership(item);
-        }
         array->setFromVector(items);
     } else {
         typeWarning("array", "ignoring attempt to replace items");
@@ -935,9 +926,12 @@ QPDFObjectHandle::setArrayFromVector(std::vector<QPDFObjectHandle> const& items)
 void
 QPDFObjectHandle::insertItem(int at, QPDFObjectHandle const& item)
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array) {
+        if (!array->insert(at, item)) {
-        array->insertItem(at, item);
+            objectWarning(
+                "ignoring attempt to insert out of bounds array item");
+            QTC::TC("qpdf", "QPDFObjectHandle insert array bounds");
+        }
     } else {
         typeWarning("array", "ignoring attempt to insert item");
         QTC::TC("qpdf", "QPDFObjectHandle array ignoring insert item");
@@ -954,10 +948,8 @@ QPDFObjectHandle::insertItemAndGetNew(int at, QPDFObjectHandle const& item)
 void
 QPDFObjectHandle::appendItem(QPDFObjectHandle const& item)
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array) {
+        array->push_back(item);
-        checkOwnership(item);
-        array->appendItem(item);
     } else {
         typeWarning("array", "ignoring attempt to append item");
         QTC::TC("qpdf", "QPDFObjectHandle array ignoring append item");
@@ -974,28 +966,23 @@ QPDFObjectHandle::appendItemAndGetNew(QPDFObjectHandle const& item)
 void
 QPDFObjectHandle::eraseItem(int at)
 {
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array && (at < array->getNItems()) && (at >= 0)) {
+        if (!array->erase(at)) {
-        array->eraseItem(at);
-    } else {
-        if (array) {
             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");
     }
 }
 
 QPDFObjectHandle
 QPDFObjectHandle::eraseItemAndGetOld(int at)
 {
-    auto result = QPDFObjectHandle::newNull();
     auto array = asArray();
-    if (array && (at < array->getNItems()) && (at >= 0)) {
+    auto result =
-        result = array->getItem(at);
+        (array && at < array->size() && at >= 0) ? array->at(at) : newNull();
-    }
     eraseItem(at);
     return result;
 }
@@ -1515,11 +1502,10 @@ QPDFObjectHandle::arrayOrStreamToStreamArray(
 {
     all_description = description;
     std::vector<QPDFObjectHandle> result;
-    auto array = asArray();
+    if (auto array = asArray()) {
-    if (array) {
+        int n_items = array->size();
-        int n_items = array->getNItems();
         for (int i = 0; i < n_items; ++i) {
-            QPDFObjectHandle item = array->getItem(i);
+            QPDFObjectHandle item = array->at(i);
             if (item.isStream()) {
                 result.push_back(item);
             } else {
@@ -2217,9 +2203,9 @@ QPDFObjectHandle::makeDirect(
     } else if (isArray()) {
         std::vector<QPDFObjectHandle> items;
         auto array = asArray();
-        int n = array->getNItems();
+        int n = array->size();
         for (int i = 0; i < n; ++i) {
-            items.push_back(array->getItem(i));
+            items.push_back(array->at(i));
             items.back().makeDirect(visited, stop_at_streams);
         }
         this->obj = QPDF_Array::create(items);

libqpdf/QPDFParser.cc

@@ -27,16 +27,15 @@ namespace
     struct StackFrame
     {
         StackFrame(std::shared_ptr<InputSource> input) :
-            offset(input->tell()),
+            offset(input->tell())
-            contents_string(""),
-            contents_offset(-1)
         {
         }
 
         std::vector<std::shared_ptr<QPDFObject>> olist;
         qpdf_offset_t offset;
-        std::string contents_string;
+        std::string contents_string{""};
-        qpdf_offset_t contents_offset;
+        qpdf_offset_t contents_offset{-1};
+        int null_count{0};
     };
 } // namespace
 
@@ -50,6 +49,7 @@ QPDFParser::parse(bool& empty, bool content_stream)
     // this, it will cause a logic error to be thrown from
     // QPDF::inParse().
 
+    const static std::shared_ptr<QPDFObject> null_oh = QPDF_Null::create();
     QPDF::ParseGuard pg(context);
 
     empty = false;
@@ -67,7 +67,6 @@ QPDFParser::parse(bool& empty, bool content_stream)
     int good_count = 0;
     bool b_contents = false;
     bool is_null = false;
-    auto null_oh = QPDF_Null::create();
 
     while (!done) {
         bool bad = false;
@@ -156,6 +155,8 @@ QPDFParser::parse(bool& empty, bool content_stream)
 
         case QPDFTokenizer::tt_null:
             is_null = true;
+            ++frame.null_count;
+
             break;
 
         case QPDFTokenizer::tt_integer:
@@ -301,9 +302,11 @@ QPDFParser::parse(bool& empty, bool content_stream)
 
         case st_dictionary:
         case st_array:
-            if (!indirect_ref && !is_null) {
+            if (is_null) {
-                // No need to set description for direct nulls - they will
+                object = null_oh;
-                // become implicit.
+                // No need to set description for direct nulls - they probably
+                // will become implicit.
+            } else if (!indirect_ref) {
                 setDescription(object, input->getLastOffset());
             }
             set_offset = true;
@@ -326,7 +329,8 @@ QPDFParser::parse(bool& empty, bool content_stream)
             parser_state_e old_state = state_stack.back();
             state_stack.pop_back();
             if (old_state == st_array) {
-                object = QPDF_Array::create(std::move(olist));
+                object = QPDF_Array::create(
+                    std::move(olist), frame.null_count > 100);
                 setDescription(object, offset - 1);
                 // The `offset` points to the next of "[".  Set the rewind
                 // offset to point to the beginning of "[". This has been
@@ -381,7 +385,7 @@ QPDFParser::parse(bool& empty, bool content_stream)
                     // Calculate value.
                     std::shared_ptr<QPDFObject> val;
                     if (iter != olist.end()) {
-                        val = *iter ? *iter : QPDF_Null::create();
+                        val = *iter;
                         ++iter;
                     } else {
                         QTC::TC("qpdf", "QPDFParser no val for last key");

libqpdf/QPDF_Array.cc

@@ -1,9 +1,43 @@
 #include <qpdf/QPDF_Array.hh>
 
-#include <qpdf/QIntC.hh>
+#include <qpdf/QPDFObjectHandle.hh>
 #include <qpdf/QPDFObject_private.hh>
-#include <qpdf/QUtil.hh>
+
-#include <stdexcept>
+static const QPDFObjectHandle null_oh = QPDFObjectHandle::newNull();
+
+inline void
+QPDF_Array::checkOwnership(QPDFObjectHandle const& item) const
+{
+    if (auto obj = item.getObjectPtr()) {
+        if (qpdf) {
+            if (auto item_qpdf = obj->getQPDF()) {
+                if (qpdf != item_qpdf) {
+                    throw std::logic_error(
+                        "Attempting to add an object from a different QPDF. "
+                        "Use QPDF::copyForeignObject to add objects from "
+                        "another file.");
+                }
+            }
+        }
+    } else {
+        throw std::logic_error(
+            "Attempting to add an uninitialized object to a QPDF_Array.");
+    }
+}
+
+QPDF_Array::QPDF_Array() :
+    QPDFValue(::ot_array, "array")
+{
+}
+
+QPDF_Array::QPDF_Array(QPDF_Array const& other) :
+    QPDFValue(::ot_array, "array"),
+    sparse(other.sparse),
+    sp_size(other.sp_size),
+    sp_elements(other.sp_elements),
+    elements(other.elements)
+{
+}
 
 QPDF_Array::QPDF_Array(std::vector<QPDFObjectHandle> const& v) :
     QPDFValue(::ot_array, "array")
@@ -11,16 +45,22 @@ QPDF_Array::QPDF_Array(std::vector<QPDFObjectHandle> const& v) :
     setFromVector(v);
 }
 
-QPDF_Array::QPDF_Array(std::vector<std::shared_ptr<QPDFObject>>&& v) :
+QPDF_Array::QPDF_Array(
-    QPDFValue(::ot_array, "array")
+    std::vector<std::shared_ptr<QPDFObject>>&& v, bool sparse) :
-{
-    setFromVector(std::move(v));
-}
-
-QPDF_Array::QPDF_Array(SparseOHArray const& items) :
     QPDFValue(::ot_array, "array"),
-    elements(items)
+    sparse(sparse)
 {
+    if (sparse) {
+        for (auto&& item: v) {
+            if (item->getTypeCode() != ::ot_null ||
+                item->getObjGen().isIndirect()) {
+                sp_elements[sp_size] = std::move(item);
+            }
+            ++sp_size;
+        }
+    } else {
+        elements = std::move(v);
+    }
 }
 
 std::shared_ptr<QPDFObject>
@@ -30,37 +70,88 @@ QPDF_Array::create(std::vector<QPDFObjectHandle> const& items)
 }
 
 std::shared_ptr<QPDFObject>
-QPDF_Array::create(std::vector<std::shared_ptr<QPDFObject>>&& items)
+QPDF_Array::create(
+    std::vector<std::shared_ptr<QPDFObject>>&& items, bool sparse)
 {
-    return do_create(new QPDF_Array(std::move(items)));
+    return do_create(new QPDF_Array(std::move(items), sparse));
-}
-
-std::shared_ptr<QPDFObject>
-QPDF_Array::create(SparseOHArray const& items)
-{
-    return do_create(new QPDF_Array(items));
 }
 
 std::shared_ptr<QPDFObject>
 QPDF_Array::copy(bool shallow)
 {
-    return create(shallow ? elements : elements.copy());
+    if (shallow) {
+        return do_create(new QPDF_Array(*this));
+    } else {
+        if (sparse) {
+            QPDF_Array* result = new QPDF_Array();
+            result->sp_size = sp_size;
+            for (auto const& element: sp_elements) {
+                auto const& obj = element.second;
+                result->sp_elements[element.first] =
+                    obj->getObjGen().isIndirect() ? obj : obj->copy();
+            }
+            return do_create(result);
+        } else {
+            std::vector<std::shared_ptr<QPDFObject>> result;
+            result.reserve(elements.size());
+            for (auto const& element: elements) {
+                result.push_back(
+                    element
+                        ? (element->getObjGen().isIndirect() ? element
+                                                             : element->copy())
+                        : element);
+            }
+            return create(std::move(result), false);
+        }
+    }
 }
 
 void
 QPDF_Array::disconnect()
 {
-    elements.disconnect();
+    if (sparse) {
+        for (auto& item: sp_elements) {
+            auto& obj = item.second;
+            if (!obj->getObjGen().isIndirect()) {
+                obj->disconnect();
+            }
+        }
+    } else {
+        for (auto& obj: elements) {
+            if (!obj->getObjGen().isIndirect()) {
+                obj->disconnect();
+            }
+        }
+    }
 }
 
 std::string
 QPDF_Array::unparse()
 {
     std::string result = "[ ";
-    size_t size = this->elements.size();
+    if (sparse) {
-    for (size_t i = 0; i < size; ++i) {
+        int next = 0;
-        result += this->elements.at(i).unparse();
+        for (auto& item: sp_elements) {
-        result += " ";
+            int key = item.first;
+            for (int j = next; j < key; ++j) {
+                result += "null ";
+            }
+            item.second->resolve();
+            auto og = item.second->getObjGen();
+            result += og.isIndirect() ? og.unparse(' ') + " R "
+                                      : item.second->unparse() + " ";
+            next = ++key;
+        }
+        for (int j = next; j < sp_size; ++j) {
+            result += "null ";
+        }
+    } else {
+        for (auto const& item: elements) {
+            item->resolve();
+            auto og = item->getObjGen();
+            result += og.isIndirect() ? og.unparse(' ') + " R "
+                                      : item->unparse() + " ";
+        }
     }
     result += "]";
     return result;
@@ -69,96 +160,157 @@ QPDF_Array::unparse()
 JSON
 QPDF_Array::getJSON(int json_version)
 {
-    JSON j = JSON::makeArray();
+    static const JSON j_null = JSON::makeNull();
-    size_t size = this->elements.size();
+    JSON j_array = JSON::makeArray();
-    for (size_t i = 0; i < size; ++i) {
+    if (sparse) {
-        j.addArrayElement(this->elements.at(i).getJSON(json_version));
+        int next = 0;
+        for (auto& item: sp_elements) {
+            int key = item.first;
+            for (int j = next; j < key; ++j) {
+                j_array.addArrayElement(j_null);
+            }
+            auto og = item.second->getObjGen();
+            j_array.addArrayElement(
+                og.isIndirect() ? JSON::makeString(og.unparse(' ') + " R")
+                                : item.second->getJSON(json_version));
+            next = ++key;
+        }
+        for (int j = next; j < sp_size; ++j) {
+            j_array.addArrayElement(j_null);
+        }
+    } else {
+        for (auto const& item: elements) {
+            auto og = item->getObjGen();
+            j_array.addArrayElement(
+                og.isIndirect() ? JSON::makeString(og.unparse(' ') + " R")
+                                : item->getJSON(json_version));
+        }
     }
-    return j;
+    return j_array;
-}
-
-int
-QPDF_Array::getNItems() const
-{
-    // This should really return a size_t, but changing it would break
-    // a lot of code.
-    return QIntC::to_int(this->elements.size());
 }
 
 QPDFObjectHandle
-QPDF_Array::getItem(int n) const
+QPDF_Array::at(int n) const noexcept
 {
-    if ((n < 0) || (n >= QIntC::to_int(elements.size()))) {
+    if (n < 0 || n >= size()) {
-        throw std::logic_error(
+        return {};
-            "INTERNAL ERROR: bounds error accessing QPDF_Array element");
+    } else if (sparse) {
-    }
+        auto const& iter = sp_elements.find(n);
-    return this->elements.at(QIntC::to_size(n));
+        return iter == sp_elements.end() ? null_oh : (*iter).second;
-}
+    } else {
-
+        return elements[size_t(n)];
-void
-QPDF_Array::getAsVector(std::vector<QPDFObjectHandle>& v) const
-{
-    size_t size = this->elements.size();
-    for (size_t i = 0; i < size; ++i) {
-        v.push_back(this->elements.at(i));
     }
 }
 
-void
+std::vector<QPDFObjectHandle>
-QPDF_Array::setItem(int n, QPDFObjectHandle const& oh)
+QPDF_Array::getAsVector() const
 {
-    this->elements.setAt(QIntC::to_size(n), oh);
+    if (sparse) {
+        std::vector<QPDFObjectHandle> v;
+        v.reserve(size_t(size()));
+        for (auto const& item: sp_elements) {
+            v.resize(size_t(item.first), null_oh);
+            v.push_back(item.second);
+        }
+        v.resize(size_t(size()), null_oh);
+        return v;
+    } else {
+        return {elements.cbegin(), elements.cend()};
+    }
+}
+
+bool
+QPDF_Array::setAt(int at, QPDFObjectHandle const& oh)
+{
+    if (at < 0 || at >= size()) {
+        return false;
+    }
+    checkOwnership(oh);
+    if (sparse) {
+        sp_elements[at] = oh.getObj();
+    } else {
+        elements[size_t(at)] = oh.getObj();
+    }
+    return true;
 }
 
 void
 QPDF_Array::setFromVector(std::vector<QPDFObjectHandle> const& v)
 {
-    this->elements = SparseOHArray();
+    elements.resize(0);
-    for (auto const& iter: v) {
+    elements.reserve(v.size());
-        this->elements.append(iter);
+    for (auto const& item: v) {
+        checkOwnership(item);
+        elements.push_back(item.getObj());
     }
 }
 
-void
+bool
-QPDF_Array::setFromVector(std::vector<std::shared_ptr<QPDFObject>>&& v)
+QPDF_Array::insert(int at, QPDFObjectHandle const& item)
 {
-    this->elements = SparseOHArray();
+    int sz = size();
-    for (auto&& item: v) {
+    if (at < 0 || at > sz) {
-        if (item) {
+        // As special case, also allow insert beyond the end
-            this->elements.append(item);
+        return false;
+    } else if (at == sz) {
+        push_back(item);
+    } else {
+        checkOwnership(item);
+        if (sparse) {
+            auto iter = sp_elements.crbegin();
+            while (iter != sp_elements.crend()) {
+                auto key = (iter++)->first;
+                if (key >= at) {
+                    auto nh = sp_elements.extract(key);
+                    ++nh.key();
+                    sp_elements.insert(std::move(nh));
+                } else {
+                    break;
+                }
+            }
+            sp_elements[at] = item.getObj();
+            ++sp_size;
         } else {
-            ++this->elements.n_elements;
+            elements.insert(elements.cbegin() + at, item.getObj());
         }
     }
+    return true;
 }
 
 void
-QPDF_Array::insertItem(int at, QPDFObjectHandle const& item)
+QPDF_Array::push_back(QPDFObjectHandle const& item)
 {
-    // As special case, also allow insert beyond the end
+    checkOwnership(item);
-    if ((at < 0) || (at > QIntC::to_int(this->elements.size()))) {
+    if (sparse) {
-        throw std::logic_error(
+        sp_elements[sp_size++] = item.getObj();
-            "INTERNAL ERROR: bounds error accessing QPDF_Array element");
+    } else {
-    }
+        elements.push_back(item.getObj());
-    this->elements.insert(QIntC::to_size(at), item);
-}
-
-void
-QPDF_Array::appendItem(QPDFObjectHandle const& item)
-{
-    this->elements.append(item);
-}
-
-void
-QPDF_Array::eraseItem(int at)
-{
-    this->elements.erase(QIntC::to_size(at));
-}
-
-void
-QPDF_Array::addExplicitElementsToList(std::list<QPDFObjectHandle>& l) const
-{
-    for (auto const& iter: this->elements) {
-        l.push_back(iter.second);
     }
 }
+
+bool
+QPDF_Array::erase(int at)
+{
+    if (at < 0 || at >= size()) {
+        return false;
+    }
+    if (sparse) {
+        auto end = sp_elements.end();
+        if (auto iter = sp_elements.lower_bound(at); iter != end) {
+            if (iter->first == at) {
+                iter++;
+                sp_elements.erase(at);
+            }
+
+            while (iter != end) {
+                auto nh = sp_elements.extract(iter++);
+                --nh.key();
+                sp_elements.insert(std::move(nh));
+            }
+        }
+        --sp_size;
+    } else {
+        elements.erase(elements.cbegin() + at);
+    }
+    return true;
+}

libqpdf/QPDF_Null.cc

@@ -50,5 +50,6 @@ QPDF_Null::unparse()
 JSON
 QPDF_Null::getJSON(int json_version)
 {
+    // If this is updated, QPDF_Array::getJSON must also be updated.
     return JSON::makeNull();
 }

libqpdf/SparseOHArray.cc

@@ -1,148 +0,0 @@
-#include <qpdf/SparseOHArray.hh>
-
-#include <qpdf/QPDFObjectHandle.hh>
-#include <qpdf/QPDFObject_private.hh>
-
-#include <stdexcept>
-
-SparseOHArray::SparseOHArray() :
-    n_elements(0)
-{
-}
-
-size_t
-SparseOHArray::size() const
-{
-    return this->n_elements;
-}
-
-void
-SparseOHArray::append(QPDFObjectHandle oh)
-{
-    if (!oh.isDirectNull()) {
-        this->elements[this->n_elements] = oh;
-    }
-    ++this->n_elements;
-}
-
-void
-SparseOHArray::append(std::shared_ptr<QPDFObject>&& obj)
-{
-    if (obj->getTypeCode() != ::ot_null || !obj->getObjGen().isIndirect()) {
-        this->elements[this->n_elements] = std::move(obj);
-    }
-    ++this->n_elements;
-}
-
-QPDFObjectHandle
-SparseOHArray::at(size_t idx) const
-{
-    if (idx >= this->n_elements) {
-        throw std::logic_error(
-            "INTERNAL ERROR: bounds error accessing SparseOHArray element");
-    }
-    auto const& iter = this->elements.find(idx);
-    if (iter == this->elements.end()) {
-        return QPDFObjectHandle::newNull();
-    } else {
-        return (*iter).second;
-    }
-}
-
-void
-SparseOHArray::remove_last()
-{
-    if (this->n_elements == 0) {
-        throw std::logic_error("INTERNAL ERROR: attempt to remove"
-                               " last item from empty SparseOHArray");
-    }
-    --this->n_elements;
-    this->elements.erase(this->n_elements);
-}
-
-void
-SparseOHArray::disconnect()
-{
-    for (auto& iter: this->elements) {
-        QPDFObjectHandle::DisconnectAccess::disconnect(iter.second);
-    }
-}
-
-void
-SparseOHArray::setAt(size_t idx, QPDFObjectHandle oh)
-{
-    if (idx >= this->n_elements) {
-        throw std::logic_error("bounds error setting item in SparseOHArray");
-    }
-    if (oh.isDirectNull()) {
-        this->elements.erase(idx);
-    } else {
-        this->elements[idx] = oh;
-    }
-}
-
-void
-SparseOHArray::erase(size_t idx)
-{
-    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;
-        }
-    }
-    this->elements = dest;
-    --this->n_elements;
-}
-
-void
-SparseOHArray::insert(size_t idx, QPDFObjectHandle oh)
-{
-    if (idx > this->n_elements) {
-        throw std::logic_error("bounds error inserting item to SparseOHArray");
-    } else if (idx == this->n_elements) {
-        // Allow inserting to the last position
-        append(oh);
-    } else {
-        decltype(this->elements) dest;
-        for (auto const& iter: this->elements) {
-            if (iter.first < idx) {
-                dest.insert(iter);
-            } else {
-                dest[iter.first + 1] = iter.second;
-            }
-        }
-        this->elements = dest;
-        this->elements[idx] = oh;
-        ++this->n_elements;
-    }
-}
-
-SparseOHArray
-SparseOHArray::copy()
-{
-    SparseOHArray result;
-    result.n_elements = this->n_elements;
-    for (auto const& element: this->elements) {
-        auto value = element.second;
-        result.elements[element.first] =
-            value.isIndirect() ? value : value.shallowCopy();
-    }
-    return result;
-}
-
-SparseOHArray::const_iterator
-SparseOHArray::begin() const
-{
-    return this->elements.begin();
-}
-
-SparseOHArray::const_iterator
-SparseOHArray::end() const
-{
-    return this->elements.end();
-}

libqpdf/qpdf/QPDF_Array.hh

@@ -3,8 +3,7 @@
 
 #include <qpdf/QPDFValue.hh>
 
-#include <qpdf/SparseOHArray.hh>
+#include <map>
-#include <list>
 #include <vector>
 
 class QPDF_Array: public QPDFValue
@@ -14,34 +13,37 @@ class QPDF_Array: public QPDFValue
     static std::shared_ptr<QPDFObject>
     create(std::vector<QPDFObjectHandle> const& items);
     static std::shared_ptr<QPDFObject>
-    create(std::vector<std::shared_ptr<QPDFObject>>&& items);
+    create(std::vector<std::shared_ptr<QPDFObject>>&& items, bool sparse);
-    static std::shared_ptr<QPDFObject> create(SparseOHArray const& items);
     virtual std::shared_ptr<QPDFObject> copy(bool shallow = false);
     virtual std::string unparse();
     virtual JSON getJSON(int json_version);
     virtual void disconnect();
 
-    int getNItems() const;
+    int
-    QPDFObjectHandle getItem(int n) const;
+    size() const noexcept
-    void getAsVector(std::vector<QPDFObjectHandle>&) const;
+    {
-
+        return sparse ? sp_size : int(elements.size());
-    void setItem(int, QPDFObjectHandle const&);
+    }
+    QPDFObjectHandle at(int n) const noexcept;
+    bool setAt(int n, QPDFObjectHandle const& oh);
+    std::vector<QPDFObjectHandle> getAsVector() const;
     void setFromVector(std::vector<QPDFObjectHandle> const& items);
-    void setFromVector(std::vector<std::shared_ptr<QPDFObject>>&& items);
+    bool insert(int at, QPDFObjectHandle const& item);
-    void insertItem(int at, QPDFObjectHandle const& item);
+    void push_back(QPDFObjectHandle const& item);
-    void appendItem(QPDFObjectHandle const& item);
+    bool erase(int at);
-    void eraseItem(int at);
-
-    // Helper methods for QPDF and QPDFObjectHandle -- these are
-    // public methods since the whole class is not part of the public
-    // API. Otherwise, these would be wrapped in accessor classes.
-    void addExplicitElementsToList(std::list<QPDFObjectHandle>&) const;
 
   private:
+    QPDF_Array();
+    QPDF_Array(QPDF_Array const&);
     QPDF_Array(std::vector<QPDFObjectHandle> const& items);
-    QPDF_Array(std::vector<std::shared_ptr<QPDFObject>>&& items);
+    QPDF_Array(std::vector<std::shared_ptr<QPDFObject>>&& items, bool sparse);
-    QPDF_Array(SparseOHArray const& items);
+
-    SparseOHArray elements;
+    void checkOwnership(QPDFObjectHandle const& item) const;
+
+    bool sparse{false};
+    int sp_size{0};
+    std::map<int, std::shared_ptr<QPDFObject>> sp_elements;
+    std::vector<std::shared_ptr<QPDFObject>> elements;
 };
 
 #endif // QPDF_ARRAY_HH

libqpdf/qpdf/SparseOHArray.hh

@@ -1,35 +0,0 @@
-#ifndef QPDF_SPARSEOHARRAY_HH
-#define QPDF_SPARSEOHARRAY_HH
-
-#include <qpdf/QPDFObjectHandle.hh>
-#include <unordered_map>
-
-class QPDF_Array;
-
-class SparseOHArray
-{
-  public:
-    SparseOHArray();
-    size_t size() const;
-    void append(QPDFObjectHandle oh);
-    void append(std::shared_ptr<QPDFObject>&& obj);
-    QPDFObjectHandle at(size_t idx) const;
-    void remove_last();
-    void setAt(size_t idx, QPDFObjectHandle oh);
-    void erase(size_t idx);
-    void insert(size_t idx, QPDFObjectHandle oh);
-    SparseOHArray copy();
-    void disconnect();
-
-    typedef std::unordered_map<size_t, QPDFObjectHandle>::const_iterator
-        const_iterator;
-    const_iterator begin() const;
-    const_iterator end() const;
-
-  private:
-    friend class QPDF_Array;
-    std::unordered_map<size_t, QPDFObjectHandle> elements;
-    size_t n_elements;
-};
-
-#endif // QPDF_SPARSEOHARRAY_HH

libtests/sparse_array.cc

@@ -1,19 +1,23 @@
 #include <qpdf/assert_test.h>
 
-#include <qpdf/SparseOHArray.hh>
+#include <qpdf/QPDFObjectHandle.hh>
+#include <qpdf/QPDFObject_private.hh>
+#include <qpdf/QPDF_Array.hh>
 #include <iostream>
 
 int
 main()
 {
-    SparseOHArray a;
+    auto obj = QPDF_Array::create({}, true);
+    QPDF_Array& a = *obj->as<QPDF_Array>();
+
     assert(a.size() == 0);
 
-    a.append(QPDFObjectHandle::parse("1"));
+    a.push_back(QPDFObjectHandle::parse("1"));
-    a.append(QPDFObjectHandle::parse("(potato)"));
+    a.push_back(QPDFObjectHandle::parse("(potato)"));
-    a.append(QPDFObjectHandle::parse("null"));
+    a.push_back(QPDFObjectHandle::parse("null"));
-    a.append(QPDFObjectHandle::parse("null"));
+    a.push_back(QPDFObjectHandle::parse("null"));
-    a.append(QPDFObjectHandle::parse("/Quack"));
+    a.push_back(QPDFObjectHandle::parse("/Quack"));
     assert(a.size() == 5);
     assert(a.at(0).isInteger() && (a.at(0).getIntValue() == 1));
     assert(a.at(1).isString() && (a.at(1).getStringValue() == "potato"));
@@ -60,20 +64,20 @@ main()
     a.setAt(4, QPDFObjectHandle::newNull());
     assert(a.at(4).isNull());
 
-    a.remove_last();
+    a.erase(a.size() - 1);
     assert(a.size() == 5);
     assert(a.at(0).isName() && (a.at(0).getName() == "/First"));
     assert(a.at(1).isInteger() && (a.at(1).getIntValue() == 1));
     assert(a.at(3).isName() && (a.at(3).getName() == "/Third"));
     assert(a.at(4).isNull());
 
-    a.remove_last();
+    a.erase(a.size() - 1);
     assert(a.size() == 4);
     assert(a.at(0).isName() && (a.at(0).getName() == "/First"));
     assert(a.at(1).isInteger() && (a.at(1).getIntValue() == 1));
     assert(a.at(3).isName() && (a.at(3).getName() == "/Third"));
 
-    a.remove_last();
+    a.erase(a.size() - 1);
     assert(a.size() == 3);
     assert(a.at(0).isName() && (a.at(0).getName() == "/First"));
     assert(a.at(1).isInteger() && (a.at(1).getIntValue() == 1));

qpdf/qpdf.testcov

@@ -303,8 +303,10 @@ QPDFObjectHandle array treating as empty 0
 QPDFObjectHandle array null for non-array 0
 QPDFObjectHandle array treating as empty vector 0
 QPDFObjectHandle array ignoring set item 0
+QPDFObjectHandle set array bounds 0
 QPDFObjectHandle array ignoring replace items 0
 QPDFObjectHandle array ignoring insert item 0
+QPDFObjectHandle insert array bounds 0
 QPDFObjectHandle array ignoring append item 0
 QPDFObjectHandle array ignoring erase item 0
 QPDFObjectHandle dictionary false for hasKey 0

qpdf/qtest/qpdf/object-types-os.out

@@ -5,6 +5,8 @@ WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 429: operatio
 WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 429: operation for array attempted on object of type integer: ignoring attempt to append item
 WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 384: ignoring attempt to erase out of bounds array item
 WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 384: ignoring attempt to erase out of bounds array item
+WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 384: ignoring attempt to insert out of bounds array item
+WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 384: ignoring attempt to set out of bounds array item
 WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 429: operation for array attempted on object of type integer: ignoring attempt to erase item
 WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 429: operation for array attempted on object of type integer: ignoring attempt to insert item
 WARNING: object-types-os.pdf object stream 1, object 7 0 at offset 429: operation for array attempted on object of type integer: ignoring attempt to replace items

qpdf/qtest/qpdf/object-types.out

@@ -5,6 +5,8 @@ WARNING: object-types.pdf, object 8 0 at offset 669: operation for array attempt
 WARNING: object-types.pdf, object 8 0 at offset 669: operation for array attempted on object of type integer: ignoring attempt to append item
 WARNING: object-types.pdf, object 8 0 at offset 717: ignoring attempt to erase out of bounds array item
 WARNING: object-types.pdf, object 8 0 at offset 717: ignoring attempt to erase out of bounds array item
+WARNING: object-types.pdf, object 8 0 at offset 717: ignoring attempt to insert out of bounds array item
+WARNING: object-types.pdf, object 8 0 at offset 717: ignoring attempt to set out of bounds array item
 WARNING: object-types.pdf, object 8 0 at offset 669: operation for array attempted on object of type integer: ignoring attempt to erase item
 WARNING: object-types.pdf, object 8 0 at offset 669: operation for array attempted on object of type integer: ignoring attempt to insert item
 WARNING: object-types.pdf, object 8 0 at offset 669: operation for array attempted on object of type integer: ignoring attempt to replace items

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

qpdf/test_driver.cc

@@ -1506,6 +1506,8 @@ test_42(QPDF& pdf, char const* arg2)
     integer.appendItem(null);
     array.eraseItem(-1);
     array.eraseItem(16059);
+    array.insertItem(42, "/Dontpanic"_qpdf);
+    array.setArrayItem(42, "/Dontpanic"_qpdf);
     integer.eraseItem(0);
     integer.insertItem(0, null);
     integer.setArrayFromVector(std::vector<QPDFObjectHandle>());
@@ -3282,6 +3284,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