NNTree: rework iterators to be more memory efficient

Keep a std::pair internal to the iterators so that operator* can return a reference and operator-> can work, and each can work without copying pairs of objects around.

NNTree: rework iterators to be more memory efficient
Keep a std::pair internal to the iterators so that operator* can return a reference and operator-> can work, and each can work without copying pairs of objects around.
Jay Berkenbilt
1 parent e7e20772
Showing 8 changed files with 228 additions and 111 deletions
include/qpdf/QPDFNameTreeObjectHelper.hh
include/qpdf/QPDFNumberTreeObjectHelper.hh
libqpdf/NNTree.cc
libqpdf/QPDF.cc
libqpdf/QPDFNameTreeObjectHelper.cc
libqpdf/QPDFNumberTreeObjectHelper.cc
libqpdf/qpdf/NNTree.hh
qpdf/test_driver.cc
@@ -75,9 +75,6 @@ class QPDFNameTreeObjectHelper: public QPDFObjectHelper
     class iterator: public std::iterator<
         std::bidirectional_iterator_tag,
-        std::pair<std::string, QPDFObjectHandle>,
-        void,
-        std::pair<std::string, QPDFObjectHandle>*,
         std::pair<std::string, QPDFObjectHandle>>
     {
         friend class QPDFNameTreeObjectHelper;
@@ -105,6 +102,8 @@ class QPDFNameTreeObjectHelper: public QPDFObjectHelper
         QPDF_DLL
         reference operator*();
         QPDF_DLL
+        pointer operator->();
+        QPDF_DLL
         bool operator==(iterator const& other) const;
         QPDF_DLL
         bool operator!=(iterator const& other) const
@@ -131,8 +130,11 @@ class QPDFNameTreeObjectHelper: public QPDFObjectHelper
         void remove();
       private:
+        void updateIValue();
+
         iterator(std::shared_ptr<NNTreeIterator> const&);
         std::shared_ptr<NNTreeIterator> impl;
+        value_type ivalue;
     };
     // The iterator looks like map iterator, so i.first is a string
@@ -94,9 +94,6 @@ class QPDFNumberTreeObjectHelper: public QPDFObjectHelper
     class iterator: public std::iterator<
         std::bidirectional_iterator_tag,
-        std::pair<numtree_number, QPDFObjectHandle>,
-        void,
-        std::pair<numtree_number, QPDFObjectHandle>*,
         std::pair<numtree_number, QPDFObjectHandle>>
     {
         friend class QPDFNumberTreeObjectHelper;
@@ -124,6 +121,8 @@ class QPDFNumberTreeObjectHelper: public QPDFObjectHelper
         QPDF_DLL
         reference operator*();
         QPDF_DLL
+        pointer operator->();
+        QPDF_DLL
         bool operator==(iterator const& other) const;
         QPDF_DLL
         bool operator!=(iterator const& other) const
@@ -150,8 +149,11 @@ class QPDFNumberTreeObjectHelper: public QPDFObjectHelper
         void remove();
       private:
+        void updateIValue();
+
         iterator(std::shared_ptr<NNTreeIterator> const&);
         std::shared_ptr<NNTreeIterator> impl;
+        value_type ivalue;
     };
     // The iterator looks like map iterator, so i.first is a string
@@ -50,6 +50,57 @@ NNTreeIterator::NNTreeIterator(NNTreeImpl&amp; impl) :
 {
 }
+void
+NNTreeIterator::updateIValue(bool allow_invalid)
+{
+    // ivalue should never be used inside the class since we return a
+    // pointer/reference to it. Every bit of code that ever changes
+    // what object the iterator points to should take care to call
+    // updateIValue. Failure to do this means that any old references
+    // to *iter will point to incorrect objects, though the next
+    // dereference of the iterator will fix it. This isn't necessarily
+    // catastrophic, but it would be confusing. The test suite
+    // attempts to exercise various cases to ensure we don't introduce
+    // that bug in the future, but sadly it's tricky to verify by
+    // reasoning about the code that this constraint is always
+    // satisfied. Whenever we update what the iterator points to, we
+    // should call setItemNumber, which calls this. If we change what
+    // the iterator in some other way, such as replacing a value or
+    // removing an item and making the iterator point at a different
+    // item in potentially the same position, we must call
+    // updateIValue as well. These cases are handled, and for good
+    // measure, we also call updateIValue in operator* and operator->.
+
+    bool okay = false;
+    if ((item_number >= 0) &&
+        this->node.isInitialized() &&
+        this->node.isDictionary())
+    {
+        auto items = this->node.getKey(impl.details.itemsKey());
+        if (this->item_number + 1 < items.getArrayNItems())
+        {
+            okay = true;
+            this->ivalue.first = items.getArrayItem(this->item_number);
+            this->ivalue.second = items.getArrayItem(1+this->item_number);
+        }
+        else
+        {
+            error(impl.qpdf, node, "update ivalue: items array is too short");
+        }
+    }
+    if (! okay)
+    {
+        if (! allow_invalid)
+        {
+            throw std::logic_error(
+                "attempt made to dereference an invalid"
+                " name/number tree iterator");
+        }
+        this->ivalue.first = QPDFObjectHandle();
+        this->ivalue.second = QPDFObjectHandle();
+    }
+}
+
 NNTreeIterator::PathElement::PathElement(
     QPDFObjectHandle const& node, int kid_number) :
     node(node),
@@ -522,6 +573,7 @@ NNTreeIterator::remove()
             // We don't have to do anything since the removed item's
             // successor now occupies its former location.
             QTC::TC("qpdf", "NNTree erased non-last item");
+            updateIValue();
         }
         else
         {
@@ -630,19 +682,15 @@ NNTreeIterator::operator--()
 NNTreeIterator::reference
 NNTreeIterator::operator*()
 {
-    if (this->item_number < 0)
-    {
-        throw std::logic_error(
-            "attempt made to dereference an invalid"
-            " name/number tree iterator");
-    }
-    auto items = this->node.getKey(impl.details.itemsKey());
-    if (items.getArrayNItems() < this->item_number + 2)
-    {
-        error(impl.qpdf, node, "operator*: items array is too short");
-    }
-    return std::make_pair(items.getArrayItem(this->item_number),
-                          items.getArrayItem(1+this->item_number));
+    updateIValue(false);
+    return this->ivalue;
+}
+
+NNTreeIterator::pointer
+NNTreeIterator::operator->()
+{
+    updateIValue(false);
+    return &(this->ivalue);
 }
 bool
@@ -660,7 +708,7 @@ NNTreeIterator::operator==(NNTreeIterator const&amp; other) const
     auto opi = other.path.begin();
     while (tpi != this->path.end())
     {
-        if ((*tpi).kid_number != (*opi).kid_number)
+        if (tpi->kid_number != opi->kid_number)
         {
             return false;
         }
@@ -679,6 +727,7 @@ NNTreeIterator::setItemNumber(QPDFObjectHandle const&amp; node, int n)
 {
     this->node = node;
     this->item_number = n;
+    updateIValue();
 }
 void
@@ -961,7 +1010,7 @@ NNTreeImpl::repair()
     auto new_node = QPDFObjectHandle::newDictionary();
     new_node.replaceKey(details.itemsKey(), QPDFObjectHandle::newArray());
     NNTreeImpl repl(details, qpdf, new_node, false);
-    for (auto i: *this)
+    for (auto const& i: *this)
     {
         repl.insert(i.first, i.second);
     }
@@ -1005,15 +1054,15 @@ NNTreeImpl::findInternal(QPDFObjectHandle key, bool return_prev_if_not_found)
         return end();
     }
     else if (first_item.valid() &&
-        details.keyValid((*first_item).first) &&
-        details.compareKeys(key, (*first_item).first) < 0)
+        details.keyValid(first_item->first) &&
+        details.compareKeys(key, first_item->first) < 0)
     {
         // Before the first key
         return end();
     }
     else if (last_item.valid() &&
-             details.keyValid((*last_item).first) &&
-             details.compareKeys(key, (*last_item).first) > 0)
+             details.keyValid(last_item->first) &&
+             details.compareKeys(key, last_item->first) > 0)
     {
         // After the last key
         if (return_prev_if_not_found)
@@ -1097,10 +1146,10 @@ NNTreeImpl::insertFirst(QPDFObjectHandle key, QPDFObjectHandle value)
     }
     items.insertItem(0, key);
     items.insertItem(1, value);
-    iter.item_number = 0;
+    iter.setItemNumber(iter.node, 0);
     iter.resetLimits(iter.node, iter.lastPathElement());
     iter.split(iter.node, iter.lastPathElement());
-    return begin();
+    return iter;
 }
 NNTreeImpl::iterator
@@ -1112,11 +1161,12 @@ NNTreeImpl::insert(QPDFObjectHandle key, QPDFObjectHandle value)
         QTC::TC("qpdf", "NNTree insert inserts first");
         return insertFirst(key, value);
     }
-    else if (details.compareKeys(key, (*iter).first) == 0)
+    else if (details.compareKeys(key, iter->first) == 0)
     {
         QTC::TC("qpdf", "NNTree insert replaces");
         auto items = iter.node.getKey(details.itemsKey());
         items.setArrayItem(iter.item_number + 1, value);
+        iter.updateIValue();
     }
     else
     {
@@ -1137,7 +1187,7 @@ NNTreeImpl::remove(QPDFObjectHandle key, QPDFObjectHandle* value)
     }
     if (value)
     {
-        *value = (*iter).second;
+        *value = iter->second;
     }
     iter.remove();
     return true;
@@ -3011,7 +3011,7 @@ QPDF::findAttachmentStreams()
         return;
     }
     QPDFNameTreeObjectHelper ef_tree(embedded_files, *this);
-    for (auto i: ef_tree)
+    for (auto const& i: ef_tree)
     {
         QPDFObjectHandle item = i.second;
         if (item.isDictionary() &&
@@ -79,6 +79,7 @@ QPDFNameTreeObjectHelper::iterator&amp;
 QPDFNameTreeObjectHelper::iterator::operator++()
 {
     ++(*impl);
+    updateIValue();
     return *this;
 }
@@ -86,14 +87,38 @@ QPDFNameTreeObjectHelper::iterator&amp;
 QPDFNameTreeObjectHelper::iterator::operator--()
 {
     --(*impl);
+    updateIValue();
     return *this;
 }
+void
+QPDFNameTreeObjectHelper::iterator::updateIValue()
+{
+    if (impl->valid())
+    {
+        auto p = *impl;
+        this->ivalue.first = p->first.getUTF8Value();
+        this->ivalue.second = p->second;
+    }
+    else
+    {
+        this->ivalue.first = "";
+        this->ivalue.second = QPDFObjectHandle();
+    }
+}
+
 QPDFNameTreeObjectHelper::iterator::reference
 QPDFNameTreeObjectHelper::iterator::operator*()
 {
-    auto p = **impl;
-    return std::make_pair(p.first.getUTF8Value(), p.second);
+    updateIValue();
+    return this->ivalue;
+}
+
+QPDFNameTreeObjectHelper::iterator::pointer
+QPDFNameTreeObjectHelper::iterator::operator->()
+{
+    updateIValue();
+    return &this->ivalue;
 }
 bool
@@ -107,12 +132,14 @@ QPDFNameTreeObjectHelper::iterator::insertAfter(
     std::string const& key, QPDFObjectHandle value)
 {
     impl->insertAfter(QPDFObjectHandle::newUnicodeString(key), value);
+    updateIValue();
 }
 void
 QPDFNameTreeObjectHelper::iterator::remove()
 {
     impl->remove();
+    updateIValue();
 }
 QPDFNameTreeObjectHelper::iterator
@@ -175,7 +202,7 @@ QPDFNameTreeObjectHelper::findObject(
     {
         return false;
     }
-    oh = (*i).second;
+    oh = i->second;
     return true;
 }
@@ -75,6 +75,7 @@ QPDFNumberTreeObjectHelper::iterator&amp;
 QPDFNumberTreeObjectHelper::iterator::operator++()
 {
     ++(*impl);
+    updateIValue();
     return *this;
 }
@@ -82,14 +83,38 @@ QPDFNumberTreeObjectHelper::iterator&amp;
 QPDFNumberTreeObjectHelper::iterator::operator--()
 {
     --(*impl);
+    updateIValue();
     return *this;
 }
+void
+QPDFNumberTreeObjectHelper::iterator::updateIValue()
+{
+    if (impl->valid())
+    {
+        auto p = *impl;
+        this->ivalue.first = p->first.getIntValue();
+        this->ivalue.second = p->second;
+    }
+    else
+    {
+        this->ivalue.first = 0;
+        this->ivalue.second = QPDFObjectHandle();
+    }
+}
+
 QPDFNumberTreeObjectHelper::iterator::reference
 QPDFNumberTreeObjectHelper::iterator::operator*()
 {
-    auto p = **impl;
-    return std::make_pair(p.first.getIntValue(), p.second);
+    updateIValue();
+    return this->ivalue;
+}
+
+QPDFNumberTreeObjectHelper::iterator::pointer
+QPDFNumberTreeObjectHelper::iterator::operator->()
+{
+    updateIValue();
+    return &this->ivalue;
 }
 bool
@@ -103,12 +128,14 @@ QPDFNumberTreeObjectHelper::iterator::insertAfter(
     numtree_number key, QPDFObjectHandle value)
 {
     impl->insertAfter(QPDFObjectHandle::newInteger(key), value);
+    updateIValue();
 }
 void
 QPDFNumberTreeObjectHelper::iterator::remove()
 {
     impl->remove();
+    updateIValue();
 }
 QPDFNumberTreeObjectHelper::iterator
@@ -162,7 +189,7 @@ QPDFNumberTreeObjectHelper::getMin()
     {
         return 0;
     }
-    return (*i).first;
+    return i->first;
 }
 QPDFNumberTreeObjectHelper::numtree_number
@@ -173,7 +200,7 @@ QPDFNumberTreeObjectHelper::getMax()
     {
         return 0;
     }
-    return (*i).first;
+    return i->first;
 }
 bool
@@ -192,7 +219,7 @@ QPDFNumberTreeObjectHelper::findObject(
     {
         return false;
     }
-    oh = (*i).second;
+    oh = i->second;
     return true;
 }
@@ -206,8 +233,8 @@ QPDFNumberTreeObjectHelper::findObjectAtOrBelow(
     {
         return false;
     }
-    oh = (*i).second;
-    offset = idx - (*i).first;
+    oh = i->second;
+    offset = idx - i->first;
     return true;
 }
@@ -6,6 +6,7 @@
 #include <iterator>
 #include <list>
+#include <memory>
 class NNTreeDetails
 {
@@ -18,9 +19,6 @@ class NNTreeDetails
 class NNTreeImpl;
 class NNTreeIterator: public std::iterator<
     std::bidirectional_iterator_tag,
-    std::pair<QPDFObjectHandle, QPDFObjectHandle>,
-    void,
-    std::pair<QPDFObjectHandle, QPDFObjectHandle>*,
     std::pair<QPDFObjectHandle, QPDFObjectHandle>>
 {
     friend class NNTreeImpl;
@@ -41,6 +39,7 @@ class NNTreeIterator: public std::iterator&lt;
         return t;
     }
     reference operator*();
+    pointer operator->();
     bool operator==(NNTreeIterator const& other) const;
     bool operator!=(NNTreeIterator const& other) const
     {
@@ -63,6 +62,7 @@ class NNTreeIterator: public std::iterator&lt;
     // ABI: for qpdf 11, make qpdf a reference
     NNTreeIterator(NNTreeImpl& impl);
+    void updateIValue(bool allow_invalid = true);
     bool deepen(QPDFObjectHandle node, bool first, bool allow_empty);
     void setItemNumber(QPDFObjectHandle const& node, int);
     void addPathElement(QPDFObjectHandle const& node, int kid_number);
@@ -79,6 +79,7 @@ class NNTreeIterator: public std::iterator&lt;
     std::list<PathElement> path;
     QPDFObjectHandle node;
     int item_number;
+    value_type ivalue;
 };
 class NNTreeImpl
@@ -1749,7 +1749,7 @@ void runtest(int n, char const* filename1, char const* arg2)
         // number-tree.pdf
         QPDFObjectHandle qtest = pdf.getTrailer().getKey("/QTest");
         QPDFNumberTreeObjectHelper ntoh(qtest, pdf);
-        for (auto iter: ntoh)
+        for (auto& iter: ntoh)
         {
             std::cout << iter.first << " "
                       << iter.second.getStringValue()
@@ -1785,32 +1785,36 @@ void runtest(int n, char const* filename1, char const* arg2)
         assert(iter1 == new1.end());
         new1.insert(1, QPDFObjectHandle::newString("1"));
         ++iter1;
-        assert((*iter1).first == 1);
+        assert((*iter1).first == 1); // exercise operator* explicitly
+        auto& iter1_val = *iter1;
         --iter1;
         assert(iter1 == new1.end());
         --iter1;
-        assert((*iter1).first == 1);
+        assert(iter1->first == 1);
+        assert(iter1_val.first == 1);
         new1.insert(2, QPDFObjectHandle::newString("2"));
         ++iter1;
-        assert((*iter1).first == 2);
+        assert(iter1->first == 2);
+        assert(iter1_val.first == 2);
         ++iter1;
         assert(iter1 == new1.end());
+        assert(! iter1_val.second.isInitialized());
         ++iter1;
-        assert((*iter1).first == 1);
+        assert(iter1->first == 1);
         --iter1;
         assert(iter1 == new1.end());
         --iter1;
-        assert((*iter1).first == 2);
+        assert(iter1->first == 2);
         std::cout << "insertAfter" << std::endl;
         auto new2 = QPDFNumberTreeObjectHelper::newEmpty(pdf);
         auto iter2 = new2.begin();
         assert(iter2 == new2.end());
         iter2.insertAfter(3, QPDFObjectHandle::newString("3!"));
-        assert((*iter2).first == 3);
+        assert(iter2->first == 3);
         iter2.insertAfter(4, QPDFObjectHandle::newString("4!"));
-        assert((*iter2).first == 4);
-        for (auto i: new2)
+        assert(iter2->first == 4);
+        for (auto& i: new2)
         {
             std::cout << i.first << " " << i.second.unparse() << std::endl;
         }
@@ -1830,7 +1834,7 @@ void runtest(int n, char const* filename1, char const* arg2)
         std::cout << "/Bad2" << std::endl;
         auto bad2 = QPDFNumberTreeObjectHelper(
             pdf.getTrailer().getKey("/Bad2"), pdf);
-        for (auto i: bad2)
+        for (auto& i: bad2)
         {
             std::cout << i.first << " " << i.second.unparse() << std::endl;
         }
@@ -1844,21 +1848,21 @@ void runtest(int n, char const* filename1, char const* arg2)
             assert(empty.begin() == empty.end());
             assert(empty.last() == empty.end());
             auto i = empty.insert(5, QPDFObjectHandle::newString("5"));
-            assert((*i).first == 5);
-            assert((*i).second.getStringValue() == "5");
-            assert((*empty.begin()).first == 5);
-            assert((*empty.last()).first == 5);
-            assert((*empty.begin()).second.getStringValue() == "5");
+            assert(i->first == 5);
+            assert(i->second.getStringValue() == "5");
+            assert(empty.begin()->first == 5);
+            assert(empty.last()->first == 5);
+            assert(empty.begin()->second.getStringValue() == "5");
             i = empty.insert(5, QPDFObjectHandle::newString("5+"));
-            assert((*i).first == 5);
-            assert((*i).second.getStringValue() == "5+");
-            assert((*empty.begin()).second.getStringValue() == "5+");
+            assert(i->first == 5);
+            assert(i->second.getStringValue() == "5+");
+            assert(empty.begin()->second.getStringValue() == "5+");
             i = empty.insert(6, QPDFObjectHandle::newString("6"));
-            assert((*i).first == 6);
-            assert((*i).second.getStringValue() == "6");
-            assert((*empty.begin()).second.getStringValue() == "5+");
-            assert((*empty.last()).first == 6);
-            assert((*empty.last()).second.getStringValue() == "6");
+            assert(i->first == 6);
+            assert(i->second.getStringValue() == "6");
+            assert(empty.begin()->second.getStringValue() == "5+");
+            assert(empty.last()->first == 6);
+            assert(empty.last()->second.getStringValue() == "6");
         }
         std::cout << "Insert into invalid" << std::endl;
         auto invalid1 = QPDFNumberTreeObjectHelper(
@@ -1875,7 +1879,7 @@ void runtest(int n, char const* filename1, char const* arg2)
         std::cout << "/Bad3, no repair" << std::endl;
         auto bad3_oh = pdf.getTrailer().getKey("/Bad3");
         auto bad3 = QPDFNumberTreeObjectHelper(bad3_oh, pdf, false);
-        for (auto i: bad3)
+        for (auto& i: bad3)
         {
             std::cout << i.first << " " << i.second.unparse() << std::endl;
         }
@@ -1883,7 +1887,7 @@ void runtest(int n, char const* filename1, char const* arg2)
         std::cout << "/Bad3, repair" << std::endl;
         bad3 = QPDFNumberTreeObjectHelper(bad3_oh, pdf, true);
-        for (auto i: bad3)
+        for (auto& i: bad3)
         {
             std::cout << i.first << " " << i.second.unparse() << std::endl;
         }
@@ -1893,7 +1897,7 @@ void runtest(int n, char const* filename1, char const* arg2)
         auto bad4 = QPDFNumberTreeObjectHelper(
             pdf.getTrailer().getKey("/Bad4"), pdf);
         bad4.insert(5, QPDFObjectHandle::newString("5"));
-        for (auto i: bad4)
+        for (auto& i: bad4)
         {
             std::cout << i.first << " " << i.second.unparse() << std::endl;
         }
@@ -1924,7 +1928,7 @@ void runtest(int n, char const* filename1, char const* arg2)
         // name-tree.pdf
         QPDFObjectHandle qtest = pdf.getTrailer().getKey("/QTest");
         QPDFNameTreeObjectHelper ntoh(qtest, pdf);
-        for (auto iter: ntoh)
+        for (auto& iter: ntoh)
         {
             std::cout << iter.first << " -> "
                       << iter.second.getStringValue()
@@ -1945,8 +1949,8 @@ void runtest(int n, char const* filename1, char const* arg2)
         assert(ntoh.findObject("07 sev\xe2\x80\xa2n", oh));
         assert("seven!" == oh.getStringValue());
         auto last = ntoh.last();
-        assert((*last).first == "29 twenty-nine");
-        assert((*last).second.getUTF8Value() == "twenty-nine!");
+        assert(last->first == "29 twenty-nine");
+        assert(last->second.getUTF8Value() == "twenty-nine!");
         auto new1 = QPDFNameTreeObjectHelper::newEmpty(pdf);
         auto iter1 = new1.begin();
@@ -1957,32 +1961,36 @@ void runtest(int n, char const* filename1, char const* arg2)
         assert(iter1 == new1.end());
         new1.insert("1", QPDFObjectHandle::newString("1"));
         ++iter1;
-        assert((*iter1).first == "1");
+        assert(iter1->first == "1");
+        auto& iter1_val = *iter1;
         --iter1;
         assert(iter1 == new1.end());
         --iter1;
-        assert((*iter1).first == "1");
+        assert(iter1->first == "1");
+        assert(iter1_val.first == "1");
         new1.insert("2", QPDFObjectHandle::newString("2"));
         ++iter1;
-        assert((*iter1).first == "2");
+        assert(iter1->first == "2");
+        assert(iter1_val.first == "2");
         ++iter1;
         assert(iter1 == new1.end());
+        assert(! iter1_val.second.isInitialized());
         ++iter1;
-        assert((*iter1).first == "1");
+        assert(iter1->first == "1");
         --iter1;
         assert(iter1 == new1.end());
         --iter1;
-        assert((*iter1).first == "2");
+        assert(iter1->first == "2");
         std::cout << "insertAfter" << std::endl;
         auto new2 = QPDFNameTreeObjectHelper::newEmpty(pdf);
         auto iter2 = new2.begin();
         assert(iter2 == new2.end());
         iter2.insertAfter("3", QPDFObjectHandle::newString("3!"));
-        assert((*iter2).first == "3");
+        assert(iter2->first == "3");
         iter2.insertAfter("4", QPDFObjectHandle::newString("4!"));
-        assert((*iter2).first == "4");
-        for (auto i: new2)
+        assert(iter2->first == "4");
+        for (auto& i: new2)
         {
             std::cout << i.first << " " << i.second.unparse() << std::endl;
         }
@@ -1996,21 +2004,21 @@ void runtest(int n, char const* filename1, char const* arg2)
             assert(empty.begin() == empty.end());
             assert(empty.last() == empty.end());
             auto i = empty.insert("five", QPDFObjectHandle::newString("5"));
-            assert((*i).first == "five");
-            assert((*i).second.getStringValue() == "5");
-            assert((*empty.begin()).first == "five");
-            assert((*empty.last()).first == "five");
-            assert((*empty.begin()).second.getStringValue() == "5");
+            assert(i->first == "five");
+            assert(i->second.getStringValue() == "5");
+            assert(empty.begin()->first == "five");
+            assert(empty.last()->first == "five");
+            assert(empty.begin()->second.getStringValue() == "5");
             i = empty.insert("five", QPDFObjectHandle::newString("5+"));
-            assert((*i).first == "five");
-            assert((*i).second.getStringValue() == "5+");
-            assert((*empty.begin()).second.getStringValue() == "5+");
+            assert(i->first == "five");
+            assert(i->second.getStringValue() == "5+");
+            assert(empty.begin()->second.getStringValue() == "5+");
             i = empty.insert("six", QPDFObjectHandle::newString("6"));
-            assert((*i).first == "six");
-            assert((*i).second.getStringValue() == "6");
-            assert((*empty.begin()).second.getStringValue() == "5+");
-            assert((*empty.last()).first == "six");
-            assert((*empty.last()).second.getStringValue() == "6");
+            assert(i->first == "six");
+            assert(i->second.getStringValue() == "6");
+            assert(empty.begin()->second.getStringValue() == "5+");
+            assert(empty.last()->first == "six");
+            assert(empty.last()->second.getStringValue() == "6");
         }
         // Exercise deprecated API until qpdf 11
@@ -2030,8 +2038,8 @@ void runtest(int n, char const* filename1, char const* arg2)
         std::cout << "/Bad1 -- wrong key type" << std::endl;
         bad1 = QPDFNameTreeObjectHelper(
             pdf.getTrailer().getKey("/Bad1"), pdf);
-        assert((*bad1.find("G", true)).first == "A");
-        for (auto i: bad1)
+        assert(bad1.find("G", true)->first == "A");
+        for (auto const& i: bad1)
         {
             std::cout << i.first << std::endl;
         }
@@ -2039,8 +2047,8 @@ void runtest(int n, char const* filename1, char const* arg2)
         std::cout << "/Bad2 -- invalid kid" << std::endl;
         auto bad2 = QPDFNameTreeObjectHelper(
             pdf.getTrailer().getKey("/Bad2"), pdf);
-        assert((*bad2.find("G", true)).first == "B");
-        for (auto i: bad2)
+        assert(bad2.find("G", true)->first == "B");
+        for (auto const& i: bad2)
         {
             std::cout << i.first << std::endl;
         }
@@ -2053,8 +2061,8 @@ void runtest(int n, char const* filename1, char const* arg2)
         std::cout << "/Bad4 -- invalid kid" << std::endl;
         auto bad4 = QPDFNameTreeObjectHelper(
             pdf.getTrailer().getKey("/Bad4"), pdf);
-        assert((*bad4.find("F", true)).first == "C");
-        for (auto i: bad4)
+        assert(bad4.find("F", true)->first == "C");
+        for (auto const& i: bad4)
         {
             std::cout << i.first << std::endl;
         }
@@ -2062,12 +2070,12 @@ void runtest(int n, char const* filename1, char const* arg2)
         std::cout << "/Bad5 -- loop in find" << std::endl;
         auto bad5 = QPDFNameTreeObjectHelper(
             pdf.getTrailer().getKey("/Bad5"), pdf);
-        assert((*bad5.find("F", true)).first == "D");
+        assert(bad5.find("F", true)->first == "D");
         std::cout << "/Bad6 -- bad limits" << std::endl;
         auto bad6 = QPDFNameTreeObjectHelper(
             pdf.getTrailer().getKey("/Bad6"), pdf);
-        assert((*bad6.insert("H", QPDFObjectHandle::newNull())).first == "H");
+        assert(bad6.insert("H", QPDFObjectHandle::newNull())->first == "H");
     }
     else if (n == 49)
     {
@@ -2574,12 +2582,12 @@ void runtest(int n, char const* filename1, char const* arg2)
         auto check_split1 = [&split1](int k) {
             auto i = split1.insert(k, QPDFObjectHandle::newString(
                                        QUtil::int_to_string(k)));
-            assert((*i).first == k);
+            assert(i->first == k);
         };
         check_split1(15);
         check_split1(35);
         check_split1(125);
-        for (auto i: split1)
+        for (auto const& i: split1)
         {
             std::cout << i.first << std::endl;
         }
@@ -2591,10 +2599,10 @@ void runtest(int n, char const* filename1, char const* arg2)
         auto check_split2 = [](QPDFNameTreeObjectHelper& noh,
                                std::string const& k) {
             auto i = noh.insert(k, QPDFObjectHandle::newUnicodeString(k));
-            assert((*i).first == k);
+            assert(i->first == k);
         };
         check_split2(split2, "C");
-        for (auto i: split2)
+        for (auto const& i: split2)
         {
             std::cout << i.first << std::endl;
         }
@@ -2605,7 +2613,7 @@ void runtest(int n, char const* filename1, char const* arg2)
         split3.setSplitThreshold(4);
         check_split2(split3, "P");
         check_split2(split3, "\xcf\x80");
-        for (auto i: split3)
+        for (auto& i: split3)
         {
             std::cout << i.first << " " << i.second.unparse() << std::endl;
         }
@@ -2626,11 +2634,11 @@ void runtest(int n, char const* filename1, char const* arg2)
         assert(value.getUTF8Value() == "c");
         auto iter1 = erase1.find("1B");
         iter1.remove();
-        assert((*iter1).first == "1D");
+        assert(iter1->first == "1D");
         iter1.remove();
         assert(iter1 == erase1.end());
         --iter1;
-        assert((*iter1).first == "1A");
+        assert(iter1->first == "1A");
         iter1.remove();
         assert(iter1 == erase1.end());
@@ -2640,14 +2648,14 @@ void runtest(int n, char const* filename1, char const* arg2)
         iter2.remove();
         assert(iter2 == erase2.end());
         --iter2;
-        assert((*iter2).first == 240);
+        assert(iter2->first == 240);
         auto k1 = erase2_oh.getKey("/Kids").getArrayItem(1);
         auto l1 = k1.getKey("/Limits");
         assert(l1.getArrayItem(0).getIntValue() == 230);
         assert(l1.getArrayItem(1).getIntValue() == 240);
         iter2 = erase2.find(210);
         iter2.remove();
-        assert((*iter2).first == 220);
+        assert(iter2->first == 220);
         k1 = erase2_oh.getKey("/Kids").getArrayItem(0);
         l1 = k1.getKey("/Limits");
         assert(l1.getArrayItem(0).getIntValue() == 220);
@@ -2667,7 +2675,7 @@ void runtest(int n, char const* filename1, char const* arg2)
             pdf.getTrailer().getKey("/Erase4"), pdf);
         iter2 = erase4.find(420);
         iter2.remove();
-        assert((*iter2).first == 430);
+        assert(iter2->first == 430);
         QPDFWriter w(pdf, "a.pdf");
         w.setStaticID(true);