From b3e6d445cbf73da2b00062c3f639c2453041ee41 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sun, 24 Jul 2022 15:42:23 -0400 Subject: [PATCH] Tweak "AndGet" mutator functions again Remove any ambiguity around whether old or new value is being returned. --- ChangeLog | 19 +++++----- TODO | 43 +++++++++++------------ include/qpdf/QPDFObjectHandle.hh | 32 +++++++++-------- libqpdf/QPDFAcroFormDocumentHelper.cc | 10 +++--- libqpdf/QPDFEFStreamObjectHelper.cc | 2 +- libqpdf/QPDFEmbeddedFileDocumentHelper.cc | 4 +-- libqpdf/QPDFJob.cc | 2 +- libqpdf/QPDFObjectHandle.cc | 22 ++++++++---- libqpdf/QPDFPageObjectHelper.cc | 6 ++-- libqpdf/QPDFWriter.cc | 4 +-- manual/release-notes.rst | 10 ++++-- qpdf/test_driver.cc | 28 +++++++++------ 12 files changed, 101 insertions(+), 81 deletions(-) diff --git a/ChangeLog b/ChangeLog index 1fddfe4f..612c359d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ 2022-07-24 Jay Berkenbilt + * 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 - - * 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 * Bug fix: "removeAttachment" in the job JSON now takes an array diff --git a/TODO b/TODO index 645b5d1f..25ca9ad7 100644 --- a/TODO +++ b/TODO @@ -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 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 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" diff --git a/include/qpdf/QPDFObjectHandle.hh b/include/qpdf/QPDFObjectHandle.hh index 790acccd..7ea6b062 100644 --- a/include/qpdf/QPDFObjectHandle.hh +++ b/include/qpdf/QPDFObjectHandle.hh @@ -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 diff --git a/libqpdf/QPDFAcroFormDocumentHelper.cc b/libqpdf/QPDFAcroFormDocumentHelper.cc index 62dbaeee..23d021ff 100644 --- a/libqpdf/QPDFAcroFormDocumentHelper.cc +++ b/libqpdf/QPDFAcroFormDocumentHelper.cc @@ -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 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 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()) { diff --git a/libqpdf/QPDFEFStreamObjectHelper.cc b/libqpdf/QPDFEFStreamObjectHelper.cc index cbfe47a3..8380206d 100644 --- a/libqpdf/QPDFEFStreamObjectHelper.cc +++ b/libqpdf/QPDFEFStreamObjectHelper.cc @@ -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); diff --git a/libqpdf/QPDFEmbeddedFileDocumentHelper.cc b/libqpdf/QPDFEmbeddedFileDocumentHelper.cc index 847a9786..fd706c27 100644 --- a/libqpdf/QPDFEmbeddedFileDocumentHelper.cc +++ b/libqpdf/QPDFEmbeddedFileDocumentHelper.cc @@ -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()) { diff --git a/libqpdf/QPDFJob.cc b/libqpdf/QPDFJob.cc index 343fa348..8e9b73a1 100644 --- a/libqpdf/QPDFJob.cc +++ b/libqpdf/QPDFJob.cc @@ -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]) { diff --git a/libqpdf/QPDFObjectHandle.cc b/libqpdf/QPDFObjectHandle.cc index 753493ec..bb816c8b 100644 --- a/libqpdf/QPDFObjectHandle.cc +++ b/libqpdf/QPDFObjectHandle.cc @@ -915,7 +915,7 @@ QPDFObjectHandle::insertItem(int at, QPDFObjectHandle const& 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& 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 og_to_name; std::set 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& key) } QPDFObjectHandle -QPDFObjectHandle::removeKeyAndGet(std::string const& key) +QPDFObjectHandle::removeKeyAndGetOld(std::string const& key) { auto result = QPDFObjectHandle::newNull(); if (isDictionary()) { diff --git a/libqpdf/QPDFPageObjectHelper.cc b/libqpdf/QPDFPageObjectHelper.cc index 96a8ce69..9ad75cf8 100644 --- a/libqpdf/QPDFPageObjectHelper.cc +++ b/libqpdf/QPDFPageObjectHelper.cc @@ -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); diff --git a/libqpdf/QPDFWriter.cc b/libqpdf/QPDFWriter.cc index 50ec9406..888c3967 100644 --- a/libqpdf/QPDFWriter.cc +++ b/libqpdf/QPDFWriter.cc @@ -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"); diff --git a/manual/release-notes.rst b/manual/release-notes.rst index 3d5b5cc5..ebbfd4f5 100644 --- a/manual/release-notes.rst +++ b/manual/release-notes.rst @@ -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: diff --git a/qpdf/test_driver.cc b/qpdf/test_driver.cc index f1c1d72a..8378c8fa 100644 --- a/qpdf/test_driver.cc +++ b/qpdf/test_driver.cc @@ -1095,8 +1095,8 @@ test_27(QPDF& 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& 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& 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& 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