Tweak "AndGet" mutator functions again

Remove any ambiguity around whether old or new value is being returned.

Tweak "AndGet" mutator functions again
Remove any ambiguity around whether old or new value is being returned.
Jay Berkenbilt
1 parent 3661f274
Showing 12 changed files with 101 additions and 81 deletions
ChangeLog
TODO
include/qpdf/QPDFObjectHandle.hh
libqpdf/QPDFAcroFormDocumentHelper.cc
libqpdf/QPDFEFStreamObjectHelper.cc
libqpdf/QPDFEmbeddedFileDocumentHelper.cc
libqpdf/QPDFJob.cc
libqpdf/QPDFObjectHandle.cc
libqpdf/QPDFPageObjectHelper.cc
libqpdf/QPDFWriter.cc
manual/release-notes.rst
qpdf/test_driver.cc
 2022-07-24  Jay Berkenbilt  <ejb@ql.org>
+        * QPDFObjectHandle: for the methods insertItem, appendItem,
+        eraseItem, replaceKey, and removeKey, add a corresponding
+        "AndGetNew" and/or "AndGetOld" methods. The ones that end with
+        "AndGetNew" return the newly added item. The ones that end with
+        "AndGetOld" return the old value. The AndGetNew methods make it
+        possible to create a new object, add it to an array or dictionary,
+        and get a handle to it all in one line. The AndGetOld methods make
+        it easier to retrieve an old value when removing or replacing it.
+
         * Thanks to m-holger for doing significant cleanup of private APIs
         and internals around QPDFObjGen and for significantly improving
         the performance of QPDFObjGen -- See #731. This includes a few
@@ -168,16 +177,6 @@
         128-bit without AES) an error rather than a warning when
         --allow-weak-crypto is not specified. Fixes #576.
-2022-04-29  Jay Berkenbilt  <ejb@ql.org>
-
-        * QPDFObjectHandle: for the methods insertItem, appendItem,
-        eraseItem, replaceKey, and removeKey, add a corresponding "AndGet"
-        method (insertItemAndGet, appendItemAndGet, eraseItemAndGet,
-        replaceKeyAndGet, and removeKeyAndGet) that returns the newly
-        inserted, replaced, or removed item. This makes it possible to
-        create a new object, add it to an array or dictionary, and get a
-        handle to it all in one line.
-
 2022-04-24  Jay Berkenbilt  <ejb@ql.org>
         * Bug fix: "removeAttachment" in the job JSON now takes an array
@@ -8,29 +8,6 @@ Before Release:
 * Stay on top of https://github.com/pikepdf/pikepdf/pull/315
 * Release qtest with updates to qtest-driver and copy back into qpdf
-Parent pointer idea:
-
-* Have replaceKey, removeKey, and eraseItem return the old values. The
-  comments will clarify the difference between these and the andGet
-  versions.
-* Add std::weak_ptr<QPDFObject> parent to QPDFObject. When adding a
-  direct object to an array or dictionary, set its parent. When
-  removing it, clear the parent pointer.
-* When a direct object that already has a parent is added to
-  something, it is a warning and will become an error in qpdf 12.
-  There needs to be unsafe add methods used by unsafeShallowCopy.
-  These will add but not modify the parent pointer.
-
-This allows an object to be moved from one object to another by
-removing it, which returns the now orphaned object, and then inserting
-it somewhere else. It also doesn't break the pattern of adding a
-direct object to something and subsequently mutating it. It just
-prevents the same object from being added to more than one thing.
-
-Note that arrays and dictionaries still need to contain
-QPDFObjectHandle because of indirect objects. This only pertains to
-direct objects, which are always "resolved" in QPDFObjectHandle.
-
 Next:
 * JSON v2 fixes
@@ -70,6 +47,26 @@ Pending changes:
   about the case of more than 65,536 pages. If the top node has more
   than 256 children, we'll live with it.
+Parent pointer idea:
+
+* Add std::weak_ptr<QPDFObject> parent to QPDFObject. When adding a
+  direct object to an array or dictionary, set its parent. When
+  removing it, clear the parent pointer.
+* When a direct object that already has a parent is added to
+  something, it is a warning and will become an error in qpdf 12.
+  There needs to be unsafe add methods used by unsafeShallowCopy.
+  These will add but not modify the parent pointer.
+
+This allows an object to be moved from one object to another by
+removing it, which returns the now orphaned object, and then inserting
+it somewhere else. It also doesn't break the pattern of adding a
+direct object to something and subsequently mutating it. It just
+prevents the same object from being added to more than one thing.
+
+Note that arrays and dictionaries still need to contain
+QPDFObjectHandle because of indirect objects. This only pertains to
+direct objects, which are always "resolved" in QPDFObjectHandle.
+
 Soon: Break ground on "Document-level work"
@@ -999,18 +999,15 @@ class QPDFObjectHandle
     // Mutator methods.
-    // Since qpdf 11: when a mutator object returns QPDFObjectHandle&,
-    // it is a reference to the object itself. This makes it possible
-    // to use a fluent style. For example:
+    // Since qpdf 11: for mutators that may add or remove an item,
+    // there are additional versions whose names contain "AndGet" that
+    // return the added or removed item. For example:
     //
-    //   array.appendItem(i1).appendItem(i2);
-    //
-    // would append i1 and then i2 to the array. There are also items
-    // that end with AndGet and return a QPDFObjectHandle. These
-    // return the newly added object. For example:
-    //
-    //   auto new_dict = dict.replaceKeyAndGet(
+    //   auto new_dict = dict.replaceKeyAndGetNew(
     //       "/New", QPDFObjectHandle::newDictionary());
+    //
+    //   auto old_value = dict.replaceKeyAndGetOld(
+    //       "/New", "(something)"_qpdf);
     // Recursively copy this object, making it direct. An exception is
     // thrown if a loop is detected. With allow_streams true, keep
@@ -1036,20 +1033,20 @@ class QPDFObjectHandle
     void insertItem(int at, QPDFObjectHandle const& item);
     // Like insertItem but return the item that was inserted.
     QPDF_DLL
-    QPDFObjectHandle insertItemAndGet(int at, QPDFObjectHandle const& item);
+    QPDFObjectHandle insertItemAndGetNew(int at, QPDFObjectHandle const& item);
     // Append an item to an array.
     QPDF_DLL
     void appendItem(QPDFObjectHandle const& item);
     // Append an item, and return the newly added item.
     QPDF_DLL
-    QPDFObjectHandle appendItemAndGet(QPDFObjectHandle const& item);
+    QPDFObjectHandle appendItemAndGetNew(QPDFObjectHandle const& item);
     // Remove the item at that position, reducing the size of the
     // array by one.
     QPDF_DLL
     void eraseItem(int at);
     // Erase and item and return the item that was removed.
     QPDF_DLL
-    QPDFObjectHandle eraseItemAndGet(int at);
+    QPDFObjectHandle eraseItemAndGetOld(int at);
     // Mutator methods for dictionary objects
@@ -1060,14 +1057,19 @@ class QPDFObjectHandle
     // Replace value of key and return the value.
     QPDF_DLL
     QPDFObjectHandle
-    replaceKeyAndGet(std::string const& key, QPDFObjectHandle const& value);
+    replaceKeyAndGetNew(std::string const& key, QPDFObjectHandle const& value);
+    // Replace value of key and return the old value, or null if the
+    // key was previously not present.
+    QPDF_DLL
+    QPDFObjectHandle
+    replaceKeyAndGetOld(std::string const& key, QPDFObjectHandle const& value);
     // Remove key, doing nothing if key does not exist.
     QPDF_DLL
     void removeKey(std::string const& key);
     // Remove key and return the old value. If the old value didn't
     // exist, return a null object.
     QPDF_DLL
-    QPDFObjectHandle removeKeyAndGet(std::string const& key);
+    QPDFObjectHandle removeKeyAndGetOld(std::string const& key);
     // ABI: Remove in qpdf 12
     [[deprecated("use replaceKey -- it does the same thing")]] QPDF_DLL void
@@ -40,7 +40,7 @@ QPDFAcroFormDocumentHelper::getOrCreateAcroForm()
 {
     auto acroform = this->qpdf.getRoot().getKey("/AcroForm");
     if (!acroform.isDictionary()) {
-        acroform = this->qpdf.getRoot().replaceKeyAndGet(
+        acroform = this->qpdf.getRoot().replaceKeyAndGetNew(
             "/AcroForm",
             this->qpdf.makeIndirectObject(QPDFObjectHandle::newDictionary()));
     }
@@ -53,8 +53,8 @@ QPDFAcroFormDocumentHelper::addFormField(QPDFFormFieldObjectHelper ff)
     auto acroform = getOrCreateAcroForm();
     auto fields = acroform.getKey("/Fields");
     if (!fields.isArray()) {
-        fields =
-            acroform.replaceKeyAndGet("/Fields", QPDFObjectHandle::newArray());
+        fields = acroform.replaceKeyAndGetNew(
+            "/Fields", QPDFObjectHandle::newArray());
     }
     fields.appendItem(ff.getObjectHandle());
     std::set<QPDFObjGen> visited;
@@ -854,7 +854,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
             }
             dr.makeResourcesIndirect(this->qpdf);
             if (!dr.isIndirect()) {
-                dr = acroform.replaceKeyAndGet(
+                dr = acroform.replaceKeyAndGetNew(
                     "/DR", this->qpdf.makeIndirectObject(dr));
             }
             // Merge the other document's /DR, creating a conflict
@@ -1076,7 +1076,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
         auto apdict = ah.getAppearanceDictionary();
         std::vector<QPDFObjectHandle> streams;
         auto replace_stream = [](auto& dict, auto& key, auto& old) {
-            return dict.replaceKeyAndGet(key, old.copyStream());
+            return dict.replaceKeyAndGetNew(key, old.copyStream());
         };
         if (apdict.isDictionary()) {
             for (auto& ap: apdict.ditems()) {
@@ -28,7 +28,7 @@ QPDFEFStreamObjectHelper::setParam(
 {
     auto params = this->oh.getDict().getKey("/Params");
     if (!params.isDictionary()) {
-        params = this->oh.getDict().replaceKeyAndGet(
+        params = this->oh.getDict().replaceKeyAndGetNew(
             "/Params", QPDFObjectHandle::newDictionary());
     }
     params.replaceKey(pkey, pval);
@@ -62,8 +62,8 @@ QPDFEmbeddedFileDocumentHelper::initEmbeddedFiles()
     auto root = qpdf.getRoot();
     auto names = root.getKey("/Names");
     if (!names.isDictionary()) {
-        names =
-            root.replaceKeyAndGet("/Names", QPDFObjectHandle::newDictionary());
+        names = root.replaceKeyAndGetNew(
+            "/Names", QPDFObjectHandle::newDictionary());
     }
     auto embedded_files = names.getKey("/EmbeddedFiles");
     if (!embedded_files.isDictionary()) {
@@ -2098,7 +2098,7 @@ QPDFJob::doUnderOverlayForPage(
     QPDFObjectHandle resources = dest_page.getAttribute("/Resources", true);
     if (!resources.isDictionary()) {
         QTC::TC("qpdf", "QPDFJob overlay page with no resources");
-        resources = dest_page.getObjectHandle().replaceKeyAndGet(
+        resources = dest_page.getObjectHandle().replaceKeyAndGetNew(
             "/Resources", QPDFObjectHandle::newDictionary());
     }
     for (int from_pageno: pagenos[pageno]) {
@@ -915,7 +915,7 @@ QPDFObjectHandle::insertItem(int at, QPDFObjectHandle const&amp; item)
 }
 QPDFObjectHandle
-QPDFObjectHandle::insertItemAndGet(int at, QPDFObjectHandle const& item)
+QPDFObjectHandle::insertItemAndGetNew(int at, QPDFObjectHandle const& item)
 {
     insertItem(at, item);
     return item;
@@ -934,7 +934,7 @@ QPDFObjectHandle::appendItem(QPDFObjectHandle const&amp; item)
 }
 QPDFObjectHandle
-QPDFObjectHandle::appendItemAndGet(QPDFObjectHandle const& item)
+QPDFObjectHandle::appendItemAndGetNew(QPDFObjectHandle const& item)
 {
     appendItem(item);
     return item;
@@ -957,7 +957,7 @@ QPDFObjectHandle::eraseItem(int at)
 }
 QPDFObjectHandle
-QPDFObjectHandle::eraseItemAndGet(int at)
+QPDFObjectHandle::eraseItemAndGetOld(int at)
 {
     auto result = QPDFObjectHandle::newNull();
     if (isArray() && (at < getArrayNItems()) && (at >= 0)) {
@@ -1113,7 +1113,8 @@ QPDFObjectHandle::mergeResources(
                     // subdictionaries just to get this shallow copy
                     // functionality.
                     QTC::TC("qpdf", "QPDFObjectHandle replace with copy");
-                    this_val = replaceKeyAndGet(rtype, this_val.shallowCopy());
+                    this_val =
+                        replaceKeyAndGetNew(rtype, this_val.shallowCopy());
                 }
                 std::map<QPDFObjGen, std::string> og_to_name;
                 std::set<std::string> rnames;
@@ -1242,13 +1243,22 @@ QPDFObjectHandle::replaceKey(
 }
 QPDFObjectHandle
-QPDFObjectHandle::replaceKeyAndGet(
+QPDFObjectHandle::replaceKeyAndGetNew(
     std::string const& key, QPDFObjectHandle const& value)
 {
     replaceKey(key, value);
     return value;
 }
+QPDFObjectHandle
+QPDFObjectHandle::replaceKeyAndGetOld(
+    std::string const& key, QPDFObjectHandle const& value)
+{
+    QPDFObjectHandle old = removeKeyAndGetOld(key);
+    replaceKey(key, value);
+    return old;
+}
+
 void
 QPDFObjectHandle::removeKey(std::string const& key)
 {
@@ -1261,7 +1271,7 @@ QPDFObjectHandle::removeKey(std::string const&amp; key)
 }
 QPDFObjectHandle
-QPDFObjectHandle::removeKeyAndGet(std::string const& key)
+QPDFObjectHandle::removeKeyAndGetOld(std::string const& key)
 {
     auto result = QPDFObjectHandle::newNull();
     if (isDictionary()) {
@@ -595,7 +595,7 @@ QPDFPageObjectHelper::removeUnreferencedResourcesHelper(
         for (auto const& iter: to_filter) {
             QPDFObjectHandle dict = resources.getKey(iter);
             if (dict.isDictionary()) {
-                dict = resources.replaceKeyAndGet(iter, dict.shallowCopy());
+                dict = resources.replaceKeyAndGetNew(iter, dict.shallowCopy());
                 rdicts.push_back(dict);
                 auto keys = dict.getKeys();
                 known_names.insert(keys.begin(), keys.end());
@@ -1110,8 +1110,8 @@ QPDFPageObjectHelper::copyAnnotations(
     afdh->addAndRenameFormFields(new_fields);
     auto annots = this->oh.getKey("/Annots");
     if (!annots.isArray()) {
-        annots =
-            this->oh.replaceKeyAndGet("/Annots", QPDFObjectHandle::newArray());
+        annots = this->oh.replaceKeyAndGetNew(
+            "/Annots", QPDFObjectHandle::newArray());
     }
     for (auto const& annot: new_annots) {
         annots.appendItem(annot);
@@ -1571,7 +1571,7 @@ QPDFWriter::unparseObject(
                         "qpdf",
                         "QPDFWriter create Extensions",
                         this->m->qdf_mode ? 0 : 1);
-                    extensions = object.replaceKeyAndGet(
+                    extensions = object.replaceKeyAndGetNew(
                         "/Extensions", QPDFObjectHandle::newDictionary());
                 }
             } else if (!have_extensions_other) {
@@ -2277,7 +2277,7 @@ QPDFWriter::prepareFileForWrite()
             if (oh.isIndirect()) {
                 QTC::TC("qpdf", "QPDFWriter make Extensions direct");
                 extensions_indirect = true;
-                oh = root.replaceKeyAndGet(key, oh.shallowCopy());
+                oh = root.replaceKeyAndGetNew(key, oh.shallowCopy());
             }
             if (oh.hasKey("/ADBE")) {
                 QPDFObjectHandle adbe = oh.getKey("/ADBE");
@@ -163,9 +163,13 @@ For a detailed list of changes, please see the file
       - See examples :file:`examples/qpdfjob-save-attachment.cc` and
         :file:`examples/qpdfjob-c-save-attachment.cc`.
-    - New methods ``insertItemAndGet``, ``appendItemAndGet``,
-      ``eraseItemAndGet``, ``replaceKeyAndGet``, and
-      ``removeKeyAndGet`` return the newly added or removed object.
+    - In ``QPDFObjectHandle``, new methods ``insertItemAndGetNew``,
+      ``appendItemAndGetNew``, and ``replaceKeyAndGetNew`` return the
+      newly added item. New methods ``eraseItemAndGetOld``,
+      ``replaceKeyAndGetOld``, and ``removeKeyAndGetOld`` return the
+      item that was just removed or, in the case of
+      ``replaceKeyAndGetOld``, a ``null`` object if the object was not
+      previously there.
     - Add new ``Pipeline`` methods to reduce the amount of casting that is
       needed:
@@ -1095,8 +1095,8 @@ test_27(QPDF&amp; pdf, char const* arg2)
         QPDFObjectHandle s2 = QPDFObjectHandle::newStream(&oldpdf, "potato\n");
         auto trailer = pdf.getTrailer();
         trailer.replaceKey("/QTest", pdf.copyForeignObject(qtest));
-        auto qtest2 =
-            trailer.replaceKeyAndGet("/QTest2", QPDFObjectHandle::newArray());
+        auto qtest2 = trailer.replaceKeyAndGetNew(
+            "/QTest2", QPDFObjectHandle::newArray());
         qtest2.appendItem(pdf.copyForeignObject(s1));
         qtest2.appendItem(pdf.copyForeignObject(s2));
         qtest2.appendItem(pdf.copyForeignObject(s3));
@@ -3165,15 +3165,23 @@ test_88(QPDF&amp; pdf, char const* arg2)
     auto dict = QPDFObjectHandle::newDictionary();
     dict.replaceKey("/One", QPDFObjectHandle::newInteger(1));
     dict.replaceKey("/Two", QPDFObjectHandle::newInteger(2));
-    auto three = dict.replaceKeyAndGet("/Three", QPDFObjectHandle::newArray());
+    auto three =
+        dict.replaceKeyAndGetNew("/Three", QPDFObjectHandle::newArray());
     three.appendItem("(a)"_qpdf);
     three.appendItem("(b)"_qpdf);
-    auto newdict = three.appendItemAndGet(QPDFObjectHandle::newDictionary());
+    auto newdict = three.appendItemAndGetNew(QPDFObjectHandle::newDictionary());
     newdict.replaceKey("/Z", "/Y"_qpdf);
     newdict.replaceKey("/X", "/W"_qpdf);
+    dict.replaceKey("/Quack", "[1 2 3]"_qpdf);
+    auto quack = dict.replaceKeyAndGetOld("/Quack", "/Moo"_qpdf);
+    assert(quack.unparse() == "[ 1 2 3 ]");
+    auto nothing =
+        dict.replaceKeyAndGetOld("/NotThere", QPDFObjectHandle::newNull());
+    assert(nothing.isNull());
     assert(dict.unparse() == R"(
       <<
         /One 1
+        /Quack /Moo
         /Two 2
         /Three [ (a) (b) << /Z /Y /X /W >> ]
       >>
@@ -3184,7 +3192,7 @@ test_88(QPDF&amp; pdf, char const* arg2)
     assert(
         arr.unparse() ==
         "[ (00) (0) (a) (b) << /Z /Y /X /W >> ]"_qpdf.unparse());
-    auto new_dict = arr.insertItemAndGet(1, "<< /P /Q /R /S >>"_qpdf);
+    auto new_dict = arr.insertItemAndGetNew(1, "<< /P /Q /R /S >>"_qpdf);
     arr.eraseItem(2);
     arr.eraseItem(0);
     assert(
@@ -3200,20 +3208,20 @@ test_88(QPDF&amp; pdf, char const* arg2)
     assert(
         arr.unparse() ==
         "[ << /P /Q /T /U >> (a) (b) << /Z /Y /X /W >> ]"_qpdf.unparse());
-    auto s = arr.eraseItemAndGet(1);
+    auto s = arr.eraseItemAndGetOld(1);
     assert(s.unparse() == "(a)");
     assert(
         arr.unparse() ==
         "[ << /P /Q /T /U >> (b) << /Z /Y /X /W >> ]"_qpdf.unparse());
-    assert(new_dict.removeKeyAndGet("/M").isNull());
-    assert(new_dict.removeKeyAndGet("/P").unparse() == "/Q");
+    assert(new_dict.removeKeyAndGetOld("/M").isNull());
+    assert(new_dict.removeKeyAndGetOld("/P").unparse() == "/Q");
     assert(new_dict.unparse() == "<< /T /U >>"_qpdf.unparse());
     // Test errors
-    auto arr2 = pdf.getRoot().replaceKeyAndGet("/QTest", "[1 2]"_qpdf);
+    auto arr2 = pdf.getRoot().replaceKeyAndGetNew("/QTest", "[1 2]"_qpdf);
     arr2.setObjectDescription(&pdf, "test array");
-    assert(arr2.eraseItemAndGet(50).isNull());
+    assert(arr2.eraseItemAndGetOld(50).isNull());
 }
 static void