Merge pull request #1532 from m-holger/oh_dict

Refactor private-API class Dictionary

Merge pull request #1532 from m-holger/oh_dict
Refactor private-API class Dictionary
m-holger · GitHub
2 parents 979dc51b 4eb9f113
Showing 8 changed files with 306 additions and 271 deletions
include/qpdf/ObjectHandle.hh
libqpdf/NNTree.cc
libqpdf/QPDF_Array.cc
libqpdf/QPDF_Dictionary.cc
libqpdf/qpdf/NNTree.hh
libqpdf/qpdf/QPDFObjectHandle_private.hh
qpdf/qpdf.testcov
qpdf/qtest/qpdf/name-tree.out
@@ -61,6 +61,13 @@ namespace qpdf
         // The rest of the header file is for qpdf internal use only.
+        // Return true if both object handles refer to the same underlying object.
+        bool
+        operator==(BaseHandle const& other) const
+        {
+            return obj == other.obj;
+        }
+
         // For arrays, return the number of items in the array.
         // For null-like objects, return 0.
         // For all other objects, return 1.
@@ -76,6 +83,10 @@ namespace qpdf
         QPDFObjectHandle operator[](size_t n) const;
         QPDFObjectHandle operator[](int n) const;
+        bool contains(std::string const& key) const;
+        size_t erase(std::string const& key);
+        QPDFObjectHandle const& operator[](std::string const& key) const;
+
         std::shared_ptr<QPDFObject> copy(bool shallow = false) const;
         // Recursively remove association with any QPDF object. This method may only be called
         // during final destruction.
@@ -57,75 +57,64 @@ NNTreeIterator::updateIValue(bool allow_invalid)
     // we must call updateIValue as well. These cases are handled, and for good measure, we also
     // call updateIValue in operator* and operator->.
-    if (item_number < 0 || !node.isDictionary()) {
+    Array items = node[impl.itemsKey()];
+    ivalue.first = items[item_number];
+    ivalue.second = items[item_number + 1];
+    if (ivalue.second) {
+        return;
+    }
+
+    if (item_number < 0 || !node) {
         if (!allow_invalid) {
             throw std::logic_error(
                 "attempt made to dereference an invalid name/number tree iterator");
         }
-        ivalue.first = QPDFObjectHandle();
-        ivalue.second = QPDFObjectHandle();
         return;
     }
-    Array items = node.getKey(impl.itemsKey());
-    if (!std::cmp_less(item_number + 1, items.size())) {
-        impl.error(node, "update ivalue: items array is too short");
-    }
-    ivalue.first = items[item_number];
-    ivalue.second = items[1 + item_number];
-}
-
-NNTreeIterator::PathElement::PathElement(QPDFObjectHandle const& node, int kid_number) :
-    node(node),
-    kid_number(kid_number)
-{
+    impl.error(node, "update ivalue: items array is too short");
 }
-QPDFObjectHandle
+Dictionary
 NNTreeIterator::getNextKid(PathElement& pe, bool backward)
 {
     while (true) {
         pe.kid_number += backward ? -1 : 1;
-        Array kids = pe.node.getKey("/Kids");
-        if (pe.kid_number >= 0 && std::cmp_less(pe.kid_number, kids.size())) {
-            auto result = kids[pe.kid_number];
-            if (result.isDictionary() &&
-                (result.hasKey("/Kids") || result.hasKey(impl.itemsKey()))) {
-                return result;
-            } else {
-                impl.warn(
-                    pe.node, "skipping over invalid kid at index " + std::to_string(pe.kid_number));
-            }
-        } else {
-            return QPDFObjectHandle::newNull();
+        Dictionary result = pe.node["/Kids"][pe.kid_number];
+        if (result.contains("/Kids") || result.contains(impl.itemsKey())) {
+            return result;
+        }
+        if (pe.kid_number < 0 || std::cmp_greater_equal(pe.kid_number, pe.node["/Kids"].size())) {
+            return {};
         }
+        impl.warn(pe.node, "skipping over invalid kid at index " + std::to_string(pe.kid_number));
     }
 }
 void
 NNTreeIterator::increment(bool backward)
 {
     if (item_number < 0) {
-        deepen(impl.oh, !backward, true);
+        deepen(impl.tree_root, !backward, true);
         return;
     }
     while (valid()) {
         item_number += backward ? -2 : 2;
-        Array items = node.getKey(impl.itemsKey());
+        Array items = node[impl.itemsKey()];
         if (item_number < 0 || std::cmp_greater_equal(item_number, items.size())) {
-            bool found = false;
             setItemNumber(QPDFObjectHandle(), -1);
-            while (!(found || path.empty())) {
+            while (!path.empty()) {
                 auto& element = path.back();
-                auto pe_node = getNextKid(element, backward);
-                if (pe_node.null()) {
-                    path.pop_back();
+                if (auto pe_node = getNextKid(element, backward)) {
+                    if (deepen(pe_node, !backward, false)) {
+                        break;
+                    }
                 } else {
-                    found = deepen(pe_node, !backward, false);
+                    path.pop_back();
                 }
             }
         }
         if (item_number >= 0) {
-            items = node.getKey(impl.itemsKey());
+            items = node[impl.itemsKey()];
             if (std::cmp_greater_equal(item_number + 1, items.size())) {
                 impl.warn(node, "items array doesn't have enough elements");
             } else if (!impl.keyValid(items[item_number])) {
@@ -140,65 +129,59 @@ NNTreeIterator::increment(bool backward)
 }
 void
-NNTreeIterator::resetLimits(QPDFObjectHandle a_node, std::list<PathElement>::iterator parent)
+NNTreeIterator::resetLimits(Dictionary a_node, std::list<PathElement>::iterator parent)
 {
     while (true) {
         if (parent == path.end()) {
-            a_node.removeKey("/Limits");
-            break;
+            a_node.erase("/Limits");
+            return;
         }
-        Array kids = a_node.getKey("/Kids");
-        size_t nkids = kids.size();
-        Array items = a_node.getKey(impl.itemsKey());
-        size_t nitems = items.size();
-        bool changed = true;
         QPDFObjectHandle first;
         QPDFObjectHandle last;
+        Array items = a_node[impl.itemsKey()];
+        size_t nitems = items.size();
         if (nitems >= 2) {
             first = items[0];
             last = items[(nitems - 1u) & ~1u];
-        } else if (nkids > 0) {
-            auto first_kid = kids[0];
-            auto last_kid = kids[nkids - 1u];
-            if (first_kid.isDictionary() && last_kid.isDictionary()) {
-                Array first_limits = first_kid.getKey("/Limits");
-                Array last_limits = last_kid.getKey("/Limits");
-                if (first_limits.size() >= 2 && last_limits.size() >= 2) {
+        } else {
+            Array kids = a_node["/Kids"];
+            size_t nkids = kids.size();
+            if (nkids > 0) {
+                Array first_limits = kids[0]["/Limits"];
+                if (first_limits.size() >= 2) {
                     first = first_limits[0];
-                    last = last_limits[1];
+                    last = kids[nkids - 1u]["/Limits"][1];
                 }
             }
         }
-        if (first && last) {
-            Array limits({first, last});
-            Array olimits = a_node.getKey("/Limits");
+        if (!(first && last)) {
+            impl.warn(a_node, "unable to determine limits");
+        } else {
+            Array olimits = a_node["/Limits"];
             if (olimits.size() == 2) {
                 auto ofirst = olimits[0];
                 auto olast = olimits[1];
                 if (impl.keyValid(ofirst) && impl.keyValid(olast) &&
                     impl.compareKeys(first, ofirst) == 0 && impl.compareKeys(last, olast) == 0) {
-                    changed = false;
+                    return;
                 }
             }
-            if (changed && !a_node.isSameObjectAs(path.begin()->node)) {
-                a_node.replaceKey("/Limits", limits);
+            if (a_node != path.begin()->node) {
+                a_node.replaceKey("/Limits", Array({first, last}));
             }
-        } else {
-            impl.warn(a_node, "unable to determine limits");
         }
-        if (!changed || parent == path.begin()) {
-            break;
-        } else {
-            a_node = parent->node;
-            --parent;
+        if (parent == path.begin()) {
+            return;
         }
+        a_node = parent->node;
+        --parent;
     }
 }
 void
-NNTreeIterator::split(QPDFObjectHandle to_split, std::list<PathElement>::iterator parent)
+NNTreeIterator::split(Dictionary to_split, std::list<PathElement>::iterator parent)
 {
     // Split some node along the path to the item pointed to by this iterator, and adjust the
     // iterator so it points to the same item.
@@ -232,9 +215,9 @@ NNTreeIterator::split(QPDFObjectHandle to_split, std::list&lt;PathElement&gt;::iterato
     }
     // Find the array we actually need to split, which is either this node's kids or items.
-    Array kids = to_split.getKey("/Kids");
+    Array kids = to_split["/Kids"];
     size_t nkids = kids.size();
-    Array items = to_split.getKey(impl.itemsKey());
+    Array items = to_split[impl.itemsKey()];
     size_t nitems = items.size();
     Array first_half;
@@ -278,12 +261,11 @@ NNTreeIterator::split(QPDFObjectHandle to_split, std::list&lt;PathElement&gt;::iterato
         // is the new first half. In this way, we make the root case identical to the non-root case
         // so remaining logic can handle them in the same way.
-        auto first_node = impl.qpdf.makeIndirectObject(QPDFObjectHandle::newDictionary());
-        first_node.replaceKey(key, first_half);
-        Array new_kids;
+        Dictionary first_node = impl.qpdf.makeIndirectObject(Dictionary({{key, first_half}}));
+        auto new_kids = Array::empty();
         new_kids.push_back(first_node);
-        to_split.removeKey("/Limits"); // already shouldn't be there for root
-        to_split.removeKey(impl.itemsKey());
+        to_split.erase("/Limits"); // already shouldn't be there for root
+        to_split.erase(impl.itemsKey());
         to_split.replaceKey("/Kids", new_kids);
         if (is_leaf) {
             node = first_node;
@@ -301,7 +283,7 @@ NNTreeIterator::split(QPDFObjectHandle to_split, std::list&lt;PathElement&gt;::iterato
     // Create a second half array, and transfer the second half of the items into the second half
     // array.
-    Array second_half;
+    auto second_half = Array::empty();
     auto start_idx = static_cast<int>((n / 2) & ~1u);
     while (std::cmp_greater(first_half.size(), start_idx)) {
         second_half.push_back(first_half[start_idx]);
@@ -310,8 +292,7 @@ NNTreeIterator::split(QPDFObjectHandle to_split, std::list&lt;PathElement&gt;::iterato
     resetLimits(to_split, parent);
     // Create a new node to contain the second half
-    QPDFObjectHandle second_node = impl.qpdf.makeIndirectObject(QPDFObjectHandle::newDictionary());
-    second_node.replaceKey(key, second_half);
+    Dictionary second_node = impl.qpdf.makeIndirectObject(Dictionary({{key, second_half}}));
     resetLimits(second_node, parent);
     // CURRENT STATE: half the items from the kids or items array in the node being split have been
@@ -322,7 +303,10 @@ NNTreeIterator::split(QPDFObjectHandle to_split, std::list&lt;PathElement&gt;::iterato
     // kid_number to traverse through it. We need to update to_split's path element, or the node if
     // this is a leaf, so that the kid/item number points to the right place.
-    Array parent_kids = parent->node.getKey("/Kids");
+    Array parent_kids = parent->node["/Kids"];
+    if (!parent_kids) {
+        impl.error(parent->node, "parent node has no /Kids array");
+    }
     parent_kids.insert(parent->kid_number + 1, second_node);
     auto cur_elem = parent;
     ++cur_elem; // points to end() for leaf nodes
@@ -347,11 +331,7 @@ NNTreeIterator::split(QPDFObjectHandle to_split, std::list&lt;PathElement&gt;::iterato
 std::list<NNTreeIterator::PathElement>::iterator
 NNTreeIterator::lastPathElement()
 {
-    auto result = path.end();
-    if (!path.empty()) {
-        --result;
-    }
-    return result;
+    return path.empty() ? path.end() : std::prev(path.end());
 }
 void
@@ -359,11 +339,11 @@ NNTreeIterator::insertAfter(QPDFObjectHandle const&amp; key, QPDFObjectHandle const&amp;
 {
     if (!valid()) {
         impl.insertFirst(key, value);
-        deepen(impl.oh, true, false);
+        deepen(impl.tree_root, true, false);
         return;
     }
-    Array items = node.getKey(impl.itemsKey());
+    Array items = node[impl.itemsKey()];
     if (!items) {
         impl.error(node, "node contains no items array");
     }
@@ -392,7 +372,7 @@ NNTreeIterator::remove()
     if (!valid()) {
         throw std::logic_error("attempt made to remove an invalid iterator");
     }
-    Array items = node.getKey(impl.itemsKey());
+    Array items = node[impl.itemsKey()];
     int nitems = static_cast<int>(items.size());
     if (std::cmp_greater(item_number + 2, nitems)) {
         impl.error(node, "found short items array while removing an item");
@@ -429,7 +409,7 @@ NNTreeIterator::remove()
     if (path.empty()) {
         // Special case: if this is the root node, we can leave it empty.
-        setItemNumber(impl.oh, -1);
+        setItemNumber(impl.tree_root, -1);
         return;
     }
@@ -439,7 +419,7 @@ NNTreeIterator::remove()
         auto element = lastPathElement();
         auto parent = element;
         --parent;
-        Array kids = element->node.getKey("/Kids");
+        Array kids = element->node["/Kids"];
         kids.erase(element->kid_number);
         auto nkids = kids.size();
         if (nkids > 0) {
@@ -465,10 +445,10 @@ NNTreeIterator::remove()
         if (parent == path.end()) {
             // We erased the very last item. Convert the root to an empty items array.
-            element->node.removeKey("/Kids");
-            element->node.replaceKey(impl.itemsKey(), Array());
+            element->node.erase("/Kids");
+            element->node.replaceKey(impl.itemsKey(), Array::empty());
             path.clear();
-            setItemNumber(impl.oh, -1);
+            setItemNumber(impl.tree_root, -1);
             return;
         }
@@ -499,14 +479,14 @@ NNTreeIterator::operator==(NNTreeIterator const&amp; other) const
 }
 bool
-NNTreeIterator::deepen(QPDFObjectHandle a_node, bool first, bool allow_empty)
+NNTreeIterator::deepen(Dictionary a_node, bool first, bool allow_empty)
 {
     // Starting at this node, descend through the first or last kid until we reach a node with
     // items. If we succeed, return true; otherwise return false and leave path alone.
     auto opath = path;
-    auto fail = [this, &opath](QPDFObjectHandle const& failed_node, std::string const& msg) {
+    auto fail = [this, &opath](Dictionary const& failed_node, std::string const& msg) {
         impl.warn(failed_node, msg);
         path = opath;
         return false;
@@ -521,58 +501,59 @@ NNTreeIterator::deepen(QPDFObjectHandle a_node, bool first, bool allow_empty)
             return fail(a_node, "loop detected while traversing name/number tree");
         }
-        if (!a_node.isDictionary()) {
+        if (!a_node) {
             return fail(a_node, "non-dictionary node while traversing name/number tree");
         }
-        Array items = a_node.getKey(impl.itemsKey());
+        Array items = a_node[impl.itemsKey()];
         int nitems = static_cast<int>(items.size());
         if (nitems > 1) {
             setItemNumber(a_node, first ? 0 : nitems - 2);
-            break;
+            return true;
         }
-        Array kids = a_node.getKey("/Kids");
+        Array kids = a_node["/Kids"];
         int nkids = static_cast<int>(kids.size());
-        if (nkids > 0) {
-            int kid_number = first ? 0 : nkids - 1;
-            addPathElement(a_node, kid_number);
-            auto next = kids[kid_number];
-            if (!next) {
-                return fail(a_node, "kid number " + std::to_string(kid_number) + " is invalid");
-            }
-            if (!next.indirect()) {
-                if (impl.auto_repair) {
-                    impl.warn(
-                        a_node,
-                        "converting kid number " + std::to_string(kid_number) +
-                            " to an indirect object");
-                    next = impl.qpdf.makeIndirectObject(next);
-                    kids.set(kid_number, next);
-                } else {
-                    impl.warn(
-                        a_node,
-                        "kid number " + std::to_string(kid_number) + " is not an indirect object");
-                }
+        if (nkids == 0) {
+            if (allow_empty && items) {
+                setItemNumber(a_node, -1);
+                return true;
             }
-            a_node = next;
-        } else if (allow_empty && items) {
-            setItemNumber(a_node, -1);
-            break;
-        } else {
             return fail(
                 a_node,
                 "name/number tree node has neither non-empty " + impl.itemsKey() + " nor /Kids");
         }
+
+        int kid_number = first ? 0 : nkids - 1;
+        addPathElement(a_node, kid_number);
+        Dictionary next = kids[kid_number];
+        if (!next) {
+            return fail(a_node, "kid number " + std::to_string(kid_number) + " is invalid");
+        }
+        if (!next.indirect()) {
+            if (impl.auto_repair) {
+                impl.warn(
+                    a_node,
+                    "converting kid number " + std::to_string(kid_number) +
+                        " to an indirect object");
+                next = impl.qpdf.makeIndirectObject(next);
+                kids.set(kid_number, next);
+            } else {
+                impl.warn(
+                    a_node,
+                    "kid number " + std::to_string(kid_number) + " is not an indirect object");
+            }
+        }
+
+        a_node = next;
     }
-    return true;
 }
 NNTreeImpl::iterator
 NNTreeImpl::begin()
 {
     iterator result(*this);
-    result.deepen(oh, true, true);
+    result.deepen(tree_root, true, true);
     return result;
 }
@@ -580,7 +561,7 @@ NNTreeImpl::iterator
 NNTreeImpl::last()
 {
     iterator result(*this);
-    result.deepen(oh, false, true);
+    result.deepen(tree_root, false, true);
     return result;
 }
@@ -606,19 +587,19 @@ NNTreeImpl::binarySearch(
     Array const& items,
     size_t num_items,
     bool return_prev_if_not_found,
-    int (NNTreeImpl::*compare)(QPDFObjectHandle const& key, Array const& arr, int item) const) const
+    bool search_kids) const
 {
     size_t max_idx = std::bit_ceil(num_items);
     int step = static_cast<int>(max_idx / 2);
-    size_t checks = max_idx;
+    int checks = static_cast<int>(std::bit_width(max_idx)); // AppImage gcc version returns size_t
     int idx = step;
     int found_idx = -1;
-    while (checks > 0) {
+    for (int i = 0; i < checks; ++i) {
         int status = -1;
         if (std::cmp_less(idx, num_items)) {
-            status = (this->*compare)(key, items, idx);
+            status = search_kids ? compareKeyKid(key, items, idx) : compareKeyItem(key, items, idx);
             if (status == 0) {
                 return idx;
             }
@@ -626,21 +607,9 @@ NNTreeImpl::binarySearch(
                 found_idx = idx;
             }
         }
-        checks >>= 1;
-        if (checks > 0) {
-            step >>= 1;
-            if (step == 0) {
-                step = 1;
-            }
-
-            if (status < 0) {
-                idx -= step;
-            } else {
-                idx += step;
-            }
-        }
+        step = std::max(step / 2, 1);
+        idx += status * step;
     }
-
     return return_prev_if_not_found ? found_idx : -1;
 }
@@ -648,7 +617,7 @@ int
 NNTreeImpl::compareKeyItem(QPDFObjectHandle const& key, Array const& items, int idx) const
 {
     if (!keyValid(items[2 * idx])) {
-        error(oh, ("item at index " + std::to_string(2 * idx) + " is not the right type"));
+        error(tree_root, ("item at index " + std::to_string(2 * idx) + " is not the right type"));
     }
     return compareKeys(key, items[2 * idx]);
 }
@@ -656,10 +625,11 @@ NNTreeImpl::compareKeyItem(QPDFObjectHandle const&amp; key, Array const&amp; items, int 
 int
 NNTreeImpl::compareKeyKid(QPDFObjectHandle const& key, Array const& kids, int idx) const
 {
-    if (!kids[idx].isDictionary()) {
-        error(oh, "invalid kid at index " + std::to_string(idx));
+    Dictionary kid = kids[idx];
+    if (!kid) {
+        error(tree_root, "invalid kid at index " + std::to_string(idx));
     }
-    Array limits = kids[idx].getKey("/Limits");
+    Array limits = kid["/Limits"];
     if (!(keyValid(limits[0]) && keyValid(limits[1]))) {
         error(kids[idx], "node is missing /Limits");
     }
@@ -675,26 +645,25 @@ NNTreeImpl::compareKeyKid(QPDFObjectHandle const&amp; key, Array const&amp; kids, int id
 void
 NNTreeImpl::repair()
 {
-    auto new_node = QPDFObjectHandle::newDictionary();
-    new_node.replaceKey(itemsKey(), Array());
+    auto new_node = Dictionary({{itemsKey(), Array::empty()}});
     NNTreeImpl repl(qpdf, new_node, key_type, value_valid, false);
     for (auto const& [key, value]: *this) {
         if (key && value) {
             repl.insert(key, value);
         }
     }
-    oh.replaceKey("/Kids", new_node.getKey("/Kids"));
-    oh.replaceKey(itemsKey(), new_node.getKey(itemsKey()));
+    tree_root.replaceKey("/Kids", new_node["/Kids"]);
+    tree_root.replaceKey(itemsKey(), new_node[itemsKey()]);
 }
 NNTreeImpl::iterator
-NNTreeImpl::find(QPDFObjectHandle key, bool return_prev_if_not_found)
+NNTreeImpl::find(QPDFObjectHandle const& key, bool return_prev_if_not_found)
 {
     try {
         return findInternal(key, return_prev_if_not_found);
     } catch (QPDFExc& e) {
         if (auto_repair) {
-            warn(oh, std::string("attempting to repair after error: ") + e.what());
+            warn(tree_root, std::string("attempting to repair after error: ") + e.what());
             repair();
             return findInternal(key, return_prev_if_not_found);
         } else {
@@ -707,26 +676,22 @@ NNTreeImpl::iterator
 NNTreeImpl::findInternal(QPDFObjectHandle const& key, bool return_prev_if_not_found)
 {
     auto first_item = begin();
-    auto last_item = end();
-    if (first_item == end()) {
+    if (!first_item.valid()) {
         return end();
     }
-    if (first_item.valid()) {
-        if (!keyValid(first_item->first)) {
-            error(oh, "encountered invalid key in find");
-        }
-        if (!value_valid(first_item->second)) {
-            error(oh, "encountered invalid value in find");
-        }
-        if (compareKeys(key, first_item->first) < 0) {
-            // Before the first key
-            return end();
-        }
+    if (!keyValid(first_item->first)) {
+        error(tree_root, "encountered invalid key in find");
+    }
+    if (!value_valid(first_item->second)) {
+        error(tree_root, "encountered invalid value in find");
+    }
+    if (compareKeys(key, first_item->first) < 0) {
+        // Before the first key
+        return end();
     }
-    qpdf_assert_debug(!last_item.valid());
     QPDFObjGen::set seen;
-    auto node = oh;
+    auto node = tree_root;
     iterator result(*this);
     while (true) {
@@ -734,35 +699,33 @@ NNTreeImpl::findInternal(QPDFObjectHandle const&amp; key, bool return_prev_if_not_fo
             error(node, "loop detected in find");
         }
-        Array items = node.getKey(itemsKey());
+        Array items = node[itemsKey()];
         size_t nitems = items.size();
         if (nitems > 1) {
-            int idx = binarySearch(
-                key, items, nitems / 2, return_prev_if_not_found, &NNTreeImpl::compareKeyItem);
+            int idx = binarySearch(key, items, nitems / 2, return_prev_if_not_found, false);
             if (idx >= 0) {
                 result.setItemNumber(node, 2 * idx);
                 if (!result.impl.keyValid(result.ivalue.first)) {
                     error(node, "encountered invalid key in find");
                 }
                 if (!result.impl.value_valid(result.ivalue.second)) {
-                    error(oh, "encountered invalid value in find");
+                    error(tree_root, "encountered invalid value in find");
                 }
             }
             return result;
         }
-        Array kids = node.getKey("/Kids");
+        Array kids = node["/Kids"];
         size_t nkids = kids.size();
-        if (nkids > 0) {
-            int idx = binarySearch(key, kids, nkids, true, &NNTreeImpl::compareKeyKid);
-            if (idx == -1) {
-                error(node, "unexpected -1 from binary search of kids; limits may by wrong");
-            }
-            result.addPathElement(node, idx);
-            node = kids[idx];
-        } else {
+        if (nkids == 0) {
             error(node, "bad node during find");
         }
+        int idx = binarySearch(key, kids, nkids, true, true);
+        if (idx == -1) {
+            error(node, "unexpected -1 from binary search of kids; limits may by wrong");
+        }
+        result.addPathElement(node, idx);
+        node = kids[idx];
     }
 }
@@ -770,18 +733,15 @@ NNTreeImpl::iterator
 NNTreeImpl::insertFirst(QPDFObjectHandle const& key, QPDFObjectHandle const& value)
 {
     auto iter = begin();
-    Array items(nullptr);
-    if (iter.node.isDictionary()) {
-        items = iter.node.getKey(itemsKey());
-    }
+    Array items = iter.node[items_key];
     if (!items) {
-        error(oh, "unable to find a valid items node");
+        error(tree_root, "unable to find a valid items node");
     }
     if (!(key && value)) {
-        error(oh, "unable to insert null key or value");
+        error(tree_root, "unable to insert null key or value");
     }
     if (!value_valid(value)) {
-        error(oh, "attempting to insert an invalid value");
+        error(tree_root, "attempting to insert an invalid value");
     }
     items.insert(0, key);
     items.insert(1, value);
@@ -798,7 +758,7 @@ NNTreeImpl::insert(QPDFObjectHandle const&amp; key, QPDFObjectHandle const&amp; value)
     if (!iter.valid()) {
         return insertFirst(key, value);
     } else if (compareKeys(key, iter->first) == 0) {
-        Array items = iter.node.getKey(itemsKey());
+        Array items = iter.node[itemsKey()];
         items.set(iter.item_number + 1, value);
         iter.updateIValue();
     } else {
@@ -829,21 +789,21 @@ NNTreeImpl::validate(bool a_repair)
     try {
         for (auto const& [key, value]: *this) {
             if (!keyValid(key)) {
-                error(oh, "invalid key in validate");
+                error(tree_root, "invalid key in validate");
             }
             if (!value_valid(value)) {
-                error(oh, "invalid value in validate");
+                error(tree_root, "invalid value in validate");
             }
             if (first) {
                 first = false;
             } else if (last_key && compareKeys(last_key, key) != -1) {
-                error(oh, "keys are not sorted in validate");
+                error(tree_root, "keys are not sorted in validate");
             }
             last_key = key;
         }
     } catch (QPDFExc& e) {
         if (a_repair) {
-            warn(oh, std::string("attempting to repair after error: ") + e.what());
+            warn(tree_root, std::string("attempting to repair after error: ") + e.what());
             repair();
         }
         return false;
@@ -61,11 +61,6 @@ Array::array() const
     return nullptr; // unreachable
 }
-Array::Array(bool empty) :
-    BaseHandle(empty ? QPDFObject::create<QPDF_Array>() : nullptr)
-{
-}
-
 Array::Array(std::vector<QPDFObjectHandle> const& items) :
     BaseHandle(QPDFObject::create<QPDF_Array>(items))
 {
@@ -76,6 +71,12 @@ Array::Array(std::vector&lt;QPDFObjectHandle&gt;&amp;&amp; items) :
 {
 }
+Array
+Array::empty()
+{
+    return Array(std::vector<QPDFObjectHandle>());
+}
+
 Array::iterator
 Array::begin()
 {
@@ -399,7 +400,6 @@ QPDFObjectHandle::getArrayItem(int n) const
             return newNull();
         }
         objectWarning("returning null for out of bounds array access");
-
     } else {
         typeWarning("array", "returning null");
     }
@@ -16,26 +16,23 @@ BaseDictionary::dict() const
     return nullptr; // unreachable
 }
-bool
-BaseDictionary::hasKey(std::string const& key) const
+QPDFObjectHandle const&
+BaseHandle::operator[](std::string const& key) const
 {
-    auto d = dict();
-    return d->items.contains(key) && !d->items[key].null();
+    if (auto d = as<QPDF_Dictionary>()) {
+        auto it = d->items.find(key);
+        if (it != d->items.end()) {
+            return it->second;
+        }
+    }
+    static const QPDFObjectHandle null_obj;
+    return null_obj;
 }
-QPDFObjectHandle
-BaseDictionary::getKey(std::string const& key) const
+bool
+BaseHandle::contains(std::string const& key) const
 {
-    auto d = dict();
-
-    // PDF spec says fetching a non-existent key from a dictionary returns the null object.
-    auto item = d->items.find(key);
-    if (item != d->items.end()) {
-        // May be a null object
-        return item->second;
-    }
-    static auto constexpr msg = " -> dictionary key $VD"sv;
-    return QPDF_Null::create(obj, msg, key);
+    return !(*this)[key].null();
 }
 std::set<std::string>
@@ -56,11 +53,14 @@ BaseDictionary::getAsMap() const
     return dict()->items;
 }
-void
-BaseDictionary::removeKey(std::string const& key)
+size_t
+BaseHandle::erase(const std::string& key)
 {
     // no-op if key does not exist
-    dict()->items.erase(key);
+    if (auto d = as<QPDF_Dictionary>()) {
+        return d->items.erase(key);
+    }
+    return 0;
 }
 void
@@ -78,13 +78,28 @@ BaseDictionary::replaceKey(std::string const&amp; key, QPDFObjectHandle value)
     }
 }
+Dictionary::Dictionary(std::map<std::string, QPDFObjectHandle>&& dict) :
+    BaseDictionary(std::move(dict))
+{
+}
+
+Dictionary::Dictionary(std::shared_ptr<QPDFObject> const& obj) :
+    BaseDictionary(obj)
+{
+}
+
+Dictionary
+Dictionary::empty()
+{
+    return Dictionary(std::map<std::string, QPDFObjectHandle>());
+}
+
 void
 QPDFObjectHandle::checkOwnership(QPDFObjectHandle const& item) const
 {
     auto qpdf = getOwningQPDF();
     auto item_qpdf = item.getOwningQPDF();
     if (qpdf && item_qpdf && qpdf != item_qpdf) {
-        QTC::TC("qpdf", "QPDFObjectHandle check ownership");
         throw std::logic_error(
             "Attempting to add an object from a different QPDF. Use "
             "QPDF::copyForeignObject to add objects from another file.");
@@ -94,9 +109,8 @@ QPDFObjectHandle::checkOwnership(QPDFObjectHandle const&amp; item) const
 bool
 QPDFObjectHandle::hasKey(std::string const& key) const
 {
-    auto dict = as_dictionary(strict);
-    if (dict) {
-        return dict.hasKey(key);
+    if (Dictionary dict = *this) {
+        return dict.contains(key);
     } else {
         typeWarning("dictionary", "returning false for a key containment request");
         QTC::TC("qpdf", "QPDFObjectHandle dictionary false for hasKey");
@@ -107,11 +121,14 @@ QPDFObjectHandle::hasKey(std::string const&amp; key) const
 QPDFObjectHandle
 QPDFObjectHandle::getKey(std::string const& key) const
 {
-    if (auto dict = as_dictionary(strict)) {
-        return dict.getKey(key);
+    if (auto result = (*this)[key]) {
+        return result;
+    }
+    if (isDictionary()) {
+        static auto constexpr msg = " -> dictionary key $VD"sv;
+        return QPDF_Null::create(obj, msg, key);
     }
     typeWarning("dictionary", "returning null for attempted key retrieval");
-    QTC::TC("qpdf", "QPDFObjectHandle dictionary null for getKey");
     static auto constexpr msg = " -> null returned from getting key $VD from non-Dictionary"sv;
     return QPDF_Null::create(obj, msg, "");
 }
@@ -174,21 +191,16 @@ QPDFObjectHandle::replaceKeyAndGetOld(std::string const&amp; key, QPDFObjectHandle c
 void
 QPDFObjectHandle::removeKey(std::string const& key)
 {
-    if (auto dict = as_dictionary(strict)) {
-        dict.removeKey(key);
+    if (erase(key) || isDictionary()) {
         return;
     }
     typeWarning("dictionary", "ignoring key removal request");
-    QTC::TC("qpdf", "QPDFObjectHandle dictionary ignoring removeKey");
 }
 QPDFObjectHandle
 QPDFObjectHandle::removeKeyAndGetOld(std::string const& key)
 {
-    auto result = QPDFObjectHandle::newNull();
-    if (auto dict = as_dictionary(strict)) {
-        result = dict.getKey(key);
-    }
-    removeKey(key);
-    return result;
+    auto result = (*this)[key];
+    erase(key);
+    return result ? result : newNull();
 }
@@ -81,9 +81,13 @@ class NNTreeIterator final
     class PathElement
     {
       public:
-        PathElement(QPDFObjectHandle const& node, int kid_number);
+        PathElement(qpdf::Dictionary const& node, int kid_number) :
+            node(node),
+            kid_number(kid_number)
+        {
+        }
-        QPDFObjectHandle node;
+        qpdf::Dictionary node;
         int kid_number;
     };
@@ -92,7 +96,7 @@ class NNTreeIterator final
     {
     }
     void updateIValue(bool allow_invalid = true);
-    bool deepen(QPDFObjectHandle node, bool first, bool allow_empty);
+    bool deepen(qpdf::Dictionary node, bool first, bool allow_empty);
     void
     setItemNumber(QPDFObjectHandle const& a_node, int n)
     {
@@ -105,16 +109,16 @@ class NNTreeIterator final
     {
         path.emplace_back(a_node, kid_number);
     }
-    QPDFObjectHandle getNextKid(PathElement& element, bool backward);
+    qpdf::Dictionary getNextKid(PathElement& element, bool backward);
     void increment(bool backward);
-    void resetLimits(QPDFObjectHandle node, std::list<PathElement>::iterator parent);
+    void resetLimits(qpdf::Dictionary node, std::list<PathElement>::iterator parent);
-    void split(QPDFObjectHandle to_split, std::list<PathElement>::iterator parent);
+    void split(qpdf::Dictionary to_split, std::list<PathElement>::iterator parent);
     std::list<PathElement>::iterator lastPathElement();
     NNTreeImpl& impl;
     std::list<PathElement> path;
-    QPDFObjectHandle node;
+    qpdf::Dictionary node;
     int item_number{-1};
     value_type ivalue;
 };
@@ -128,12 +132,12 @@ class NNTreeImpl final
     NNTreeImpl(
         QPDF& qpdf,
-        QPDFObjectHandle& oh,
+        qpdf::Dictionary tree_root,
         qpdf_object_type_e key_type,
         std::function<bool(QPDFObjectHandle const&)> value_validator,
         bool auto_repair) :
         qpdf(qpdf),
-        oh(oh),
+        tree_root(std::move(tree_root)),
         key_type(key_type),
         items_key(key_type == ::ot_string ? "/Names" : "/Nums"),
         value_valid(value_validator),
@@ -147,7 +151,7 @@ class NNTreeImpl final
         return {*this};
     }
     iterator last();
-    iterator find(QPDFObjectHandle key, bool return_prev_if_not_found = false);
+    iterator find(QPDFObjectHandle const& key, bool return_prev_if_not_found = false);
     iterator insertFirst(QPDFObjectHandle const& key, QPDFObjectHandle const& value);
     iterator insert(QPDFObjectHandle const& key, QPDFObjectHandle const& value);
     bool remove(QPDFObjectHandle const& key, QPDFObjectHandle* value = nullptr);
@@ -170,8 +174,7 @@ class NNTreeImpl final
         qpdf::Array const& items,
         size_t num_items,
         bool return_prev_if_not_found,
-        int (NNTreeImpl::*compare)(QPDFObjectHandle const& key, qpdf::Array const& arr, int item)
-            const) const;
+        bool search_kids) const;
     int compareKeyItem(QPDFObjectHandle const& key, qpdf::Array const& items, int idx) const;
     int compareKeyKid(QPDFObjectHandle const& key, qpdf::Array const& items, int idx) const;
     void warn(QPDFObjectHandle const& node, std::string const& msg);
@@ -191,7 +194,7 @@ class NNTreeImpl final
     QPDF& qpdf;
     int split_threshold{32};
-    QPDFObjectHandle oh;
+    qpdf::Dictionary tree_root;
     const qpdf_object_type_e key_type;
     const std::string items_key;
     const std::function<bool(QPDFObjectHandle const&)> value_valid;
@@ -12,18 +12,19 @@ namespace qpdf
     class Array final: public BaseHandle
     {
       public:
-        // Create an empty Array. If 'empty' is false, create a null Array.
-        Array(bool empty = true);
+        Array() = default;
-        Array(std::vector<QPDFObjectHandle> const& items);
+        explicit Array(std::vector<QPDFObjectHandle> const& items);
-        Array(std::vector<QPDFObjectHandle>&& items);
+        explicit Array(std::vector<QPDFObjectHandle>&& items);
         Array(Array const& other) :
             BaseHandle(other.obj)
         {
         }
+        static Array empty();
+
         Array&
         operator=(Array const& other)
         {
@@ -118,6 +119,11 @@ namespace qpdf
     class BaseDictionary: public BaseHandle
     {
       public:
+        // The following methods are not part of the public API.
+        std::set<std::string> getKeys();
+        std::map<std::string, QPDFObjectHandle> const& getAsMap() const;
+        void replaceKey(std::string const& key, QPDFObjectHandle value);
+
         using iterator = std::map<std::string, QPDFObjectHandle>::iterator;
         using const_iterator = std::map<std::string, QPDFObjectHandle>::const_iterator;
         using reverse_iterator = std::map<std::string, QPDFObjectHandle>::reverse_iterator;
@@ -196,41 +202,87 @@ namespace qpdf
             return {};
         }
-        // The following methods are not part of the public API.
-        bool hasKey(std::string const& key) const;
-        QPDFObjectHandle getKey(std::string const& key) const;
-        std::set<std::string> getKeys();
-        std::map<std::string, QPDFObjectHandle> const& getAsMap() const;
-        void removeKey(std::string const& key);
-        void replaceKey(std::string const& key, QPDFObjectHandle value);
-
       protected:
         BaseDictionary() = default;
-        BaseDictionary(std::shared_ptr<QPDFObject> const& obj) :
-            BaseHandle(obj) {};
-        BaseDictionary(std::shared_ptr<QPDFObject>&& obj) :
-            BaseHandle(std::move(obj)) {};
+
+        explicit BaseDictionary(std::map<std::string, QPDFObjectHandle> const& dict) :
+            BaseHandle(QPDFObject::create<QPDF_Dictionary>(dict))
+        {
+        }
+
+        explicit BaseDictionary(std::map<std::string, QPDFObjectHandle>&& dict) :
+            BaseHandle(QPDFObject::create<QPDF_Dictionary>(std::move(dict)))
+        {
+        }
+
+        explicit BaseDictionary(std::shared_ptr<QPDFObject> const& obj) :
+            BaseHandle(obj)
+        {
+        }
+        explicit BaseDictionary(std::shared_ptr<QPDFObject>&& obj) :
+            BaseHandle(std::move(obj))
+        {
+        }
         BaseDictionary(BaseDictionary const&) = default;
         BaseDictionary& operator=(BaseDictionary const&) = default;
         BaseDictionary(BaseDictionary&&) = default;
         BaseDictionary& operator=(BaseDictionary&&) = default;
+
+        explicit BaseDictionary(QPDFObjectHandle const& oh) :
+            BaseHandle(oh.resolved_type_code() == ::ot_dictionary ? oh : QPDFObjectHandle())
+        {
+        }
+
+        explicit BaseDictionary(QPDFObjectHandle&& oh) :
+            BaseHandle(
+                oh.resolved_type_code() == ::ot_dictionary ? std::move(oh) : QPDFObjectHandle())
+        {
+        }
         ~BaseDictionary() = default;
         QPDF_Dictionary* dict() const;
     };
+    // Dictionary only defines con/destructors. All other methods are inherited from BaseDictionary.
     class Dictionary final: public BaseDictionary
     {
       public:
-        explicit Dictionary(std::shared_ptr<QPDFObject> const& obj) :
-            BaseDictionary(obj)
+        Dictionary() = default;
+        explicit Dictionary(std::map<std::string, QPDFObjectHandle>&& dict);
+        explicit Dictionary(std::shared_ptr<QPDFObject> const& obj);
+
+        static Dictionary empty();
+
+        Dictionary(Dictionary const&) = default;
+        Dictionary& operator=(Dictionary const&) = default;
+        Dictionary(Dictionary&&) = default;
+        Dictionary& operator=(Dictionary&&) = default;
+
+        Dictionary(QPDFObjectHandle const& oh) :
+            BaseDictionary(oh)
         {
         }
-        explicit Dictionary(std::shared_ptr<QPDFObject>&& obj) :
-            BaseDictionary(std::move(obj))
+        Dictionary&
+        operator=(QPDFObjectHandle const& oh)
+        {
+            assign(::ot_dictionary, oh);
+            return *this;
+        }
+
+        Dictionary(QPDFObjectHandle&& oh) :
+            BaseDictionary(std::move(oh))
         {
         }
+
+        Dictionary&
+        operator=(QPDFObjectHandle&& oh)
+        {
+            assign(::ot_dictionary, std::move(oh));
+            return *this;
+        }
+
+        ~Dictionary() = default;
     };
     class Name final: public BaseHandle
@@ -304,11 +304,9 @@ QPDFObjectHandle insert array bounds 0
 QPDFObjectHandle array ignoring append item 0
 QPDFObjectHandle array ignoring erase item 0
 QPDFObjectHandle dictionary false for hasKey 0
-QPDFObjectHandle dictionary null for getKey 0
 QPDFObjectHandle dictionary empty set for getKeys 0
 QPDFObjectHandle dictionary empty map for asMap 0
 QPDFObjectHandle dictionary ignoring replaceKey 0
-QPDFObjectHandle dictionary ignoring removeKey 0
 QPDFObjectHandle numeric non-numeric 0
 QPDFObjectHandle erase array bounds 0
 qpdf-c called qpdf_check_pdf 0
@@ -537,7 +535,6 @@ QPDFWriter preserve object streams 0
 QPDFWriter preserve object streams preserve unreferenced 0
 QPDFWriter exclude from object stream 0
 QPDF_pages findPage not found 0
-QPDFObjectHandle check ownership 0
 QPDFJob weak crypto error 0
 qpdf-c called qpdf_oh_is_initialized 0
 qpdf-c registered progress reporter 0
@@ -33,7 +33,7 @@ WARNING: name-tree.pdf (Name/Number tree node (object 17)): skipping over invali
 B
 W
 /Bad3 -- invalid kid
-WARNING: name-tree.pdf (Name/Number tree node (object 25)): non-dictionary node while traversing name/number tree
+WARNING: name-tree.pdf (Name/Number tree node (object 22)): kid number 0 is invalid
 /Bad4 -- invalid kid
 WARNING: name-tree.pdf (Name/Number tree node (object 23)): attempting to repair after error: name-tree.pdf (Name/Number tree node (object 23)): invalid kid at index 1
 WARNING: name-tree.pdf (Name/Number tree node (object 23)): skipping over invalid kid at index 1