Refactor `QPDFObjectHandle` warnings: replace `warnIfPossible` with centralized …

…`warn`, streamline condition handling.

Refactor `QPDFObjectHandle` warnings: replace `warnIfPossible` with centralized …
…`warn`, streamline condition handling.
m-holger
1 parent decc7d3c
Showing 10 changed files with 75 additions and 87 deletions
libqpdf/QPDFAcroFormDocumentHelper.cc
libqpdf/QPDFEFStreamObjectHelper.cc
libqpdf/QPDFFileSpecObjectHelper.cc
libqpdf/QPDFFormFieldObjectHelper.cc
libqpdf/QPDFObjectHandle.cc
libqpdf/QPDFPageDocumentHelper.cc
libqpdf/QPDFPageObjectHelper.cc
libqpdf/QPDFWriter.cc
libqpdf/QPDF_pages.cc
qpdf/qpdf.testcov
@@ -8,6 +8,7 @@
 #include <qpdf/ResourceFinder.hh>
 using namespace qpdf;
+using namespace std::literals;
 QPDFAcroFormDocumentHelper::QPDFAcroFormDocumentHelper(QPDF& qpdf) :
     QPDFDocumentHelper(qpdf),
@@ -251,7 +252,7 @@ QPDFAcroFormDocumentHelper::analyze()
         }
     } else {
         QTC::TC("qpdf", "QPDFAcroFormDocumentHelper fields not array");
-        acroform.warnIfPossible("/Fields key of /AcroForm dictionary is not an array; ignoring");
+        acroform.warn("/Fields key of /AcroForm dictionary is not an array; ignoring");
         fields = QPDFObjectHandle::newArray();
     }
@@ -273,9 +274,9 @@ QPDFAcroFormDocumentHelper::analyze()
                 // case such as a PDF creator adding a self-contained annotation (merged with the
                 // field dictionary) to the page's /Annots array and forgetting to also put it in
                 // /AcroForm.
-                annot.warnIfPossible(
-                    "this widget annotation is not"
-                    " reachable from /AcroForm in the document catalog");
+                annot.warn(
+                    "this widget annotation is not reachable from /AcroForm in the document "
+                    "catalog");
                 m->annotation_to_field[og] = QPDFFormFieldObjectHelper(annot);
                 m->field_to_annotations[og].emplace_back(annot);
             }
@@ -292,16 +293,16 @@ QPDFAcroFormDocumentHelper::traverseField(
         // could cause stack overflow.
         return;
     }
-    if (!field.isIndirect()) {
+    if (!field.indirect()) {
         QTC::TC("qpdf", "QPDFAcroFormDocumentHelper direct field");
-        field.warnIfPossible(
+        field.warn(
             "encountered a direct object as a field or annotation while "
             "traversing /AcroForm; ignoring field or annotation");
         return;
     }
     if (!field.isDictionary()) {
         QTC::TC("qpdf", "QPDFAcroFormDocumentHelper non-dictionary field");
-        field.warnIfPossible(
+        field.warn(
             "encountered a non-dictionary as a field or annotation while"
             " traversing /AcroForm; ignoring field or annotation");
         return;
@@ -309,7 +310,7 @@ QPDFAcroFormDocumentHelper::traverseField(
     QPDFObjGen og(field.getObjGen());
     if (!visited.add(og)) {
         QTC::TC("qpdf", "QPDFAcroFormDocumentHelper loop");
-        field.warnIfPossible("loop detected while traversing /AcroForm");
+        field.warn("loop detected while traversing /AcroForm");
         return;
     }
@@ -375,7 +376,7 @@ QPDFAcroFormDocumentHelper::setNeedAppearances(bool val)
 {
     QPDFObjectHandle acroform = qpdf.getRoot().getKey("/AcroForm");
     if (!acroform.isDictionary()) {
-        qpdf.getRoot().warnIfPossible(
+        qpdf.getRoot().warn(
             "ignoring call to QPDFAcroFormDocumentHelper::setNeedAppearances"
             " on a file that lacks an /AcroForm dictionary");
         return;
@@ -592,9 +593,7 @@ QPDFAcroFormDocumentHelper::adjustDefaultAppearances(
     } catch (std::exception& e) {
         // No way to reproduce in test suite right now since error conditions are converted to
         // warnings.
-        obj.warnIfPossible(
-            std::string("Unable to parse /DA: ") + e.what() +
-            "; this form field may not update properly");
+        obj.warn("Unable to parse /DA: "s + e.what() + "; this form field may not update properly");
         return;
     }
@@ -677,16 +676,17 @@ QPDFAcroFormDocumentHelper::adjustAppearanceStream(
     try {
         auto nwarnings = qpdf.numWarnings();
         stream.parseAsContents(&rf);
-        if (qpdf.numWarnings() > nwarnings) {
-            QTC::TC("qpdf", "QPDFAcroFormDocumentHelper AP parse error");
-        }
+        QTC::TC(
+            "qpdf",
+            "QPDFAcroFormDocumentHelper AP parse error",
+            qpdf.numWarnings() > nwarnings ? 0 : 1);
         auto rr = new ResourceReplacer(dr_map, rf.getNamesByResourceType());
         auto tf = std::shared_ptr<QPDFObjectHandle::TokenFilter>(rr);
         stream.addTokenFilter(tf);
     } catch (std::exception& e) {
         // No way to reproduce in test suite right now since error conditions are converted to
         // warnings.
-        stream.warnIfPossible(std::string("Unable to parse appearance stream: ") + e.what());
+        stream.warn("Unable to parse appearance stream: "s + e.what());
     }
 }
@@ -814,7 +814,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
     QPDFObjGen::set added_new_fields;
     for (auto annot: old_annots.aitems()) {
         if (annot.isStream()) {
-            annot.warnIfPossible("ignoring annotation that's a stream");
+            annot.warn("ignoring annotation that's a stream");
             continue;
         }
@@ -847,10 +847,10 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
         bool have_field = false;
         bool have_parent = false;
         if (ffield_oh.isStream()) {
-            ffield_oh.warnIfPossible("ignoring form field that's a stream");
+            ffield.warn("ignoring form field that's a stream");
         } else if ((!ffield_oh.isNull()) && (!ffield_oh.isIndirect())) {
-            ffield_oh.warnIfPossible("ignoring form field not indirect");
-        } else if (!ffield_oh.isNull()) {
+            ffield.warn("ignoring form field not indirect");
+        } else if (!ffield.null()) {
             // A field and its associated annotation can be the same object. This matters because we
             // don't want to clone the annotation and field separately in this case.
             have_field = true;
@@ -888,7 +888,7 @@ QPDFAcroFormDocumentHelper::transformAnnotations(
                         if (orig_to_copy.contains(parent_og)) {
                             obj.replaceKey("/Parent", orig_to_copy[parent_og]);
                         } else {
-                            parent.warnIfPossible(
+                            parent.warn(
                                 "while traversing field " + obj.getObjGen().unparse(',') +
                                 ", found parent (" + parent_og.unparse(',') +
                                 ") that had not been seen, indicating likely invalid field "
@@ -138,7 +138,7 @@ QPDFEFStreamObjectHelper::newFromStream(QPDFObjectHandle stream)
     Pl_MD5 md5("EF md5", &discard);
     Pl_Count count("EF size", &md5);
     if (!stream.pipeStreamData(&count, nullptr, 0, qpdf_dl_all)) {
-        stream.warnIfPossible("unable to get stream data for new embedded file stream");
+        stream.warn("unable to get stream data for new embedded file stream");
     } else {
         result.setParam("/Size", QPDFObjectHandle::newInteger(count.getCount()));
         result.setParam(
@@ -4,32 +4,33 @@
 #include <qpdf/QTC.hh>
 #include <qpdf/QUtil.hh>
+#include <array>
 #include <string>
-#include <vector>
+
+using namespace std::literals;
 QPDFFileSpecObjectHelper::QPDFFileSpecObjectHelper(QPDFObjectHandle oh) :
     QPDFObjectHelper(oh)
 {
     if (!oh.isDictionary()) {
-        oh.warnIfPossible("Embedded file object is not a dictionary");
+        warn("Embedded file object is not a dictionary");
         return;
     }
     if (!oh.isDictionaryOfType("/Filespec")) {
-        oh.warnIfPossible("Embedded file object's type is not /Filespec");
+        warn("Embedded file object's type is not /Filespec");
     }
 }
-static std::vector<std::string> name_keys = {"/UF", "/F", "/Unix", "/DOS", "/Mac"};
+static const std::array<std::string, 5> name_keys = {"/UF"s, "/F"s, "/Unix"s, "/DOS"s, "/Mac"s};
 std::string
 QPDFFileSpecObjectHelper::getDescription()
 {
-    std::string result;
     auto desc = oh().getKey("/Desc");
     if (desc.isString()) {
-        result = desc.getUTF8Value();
+        return desc.getUTF8Value();
     }
-    return result;
+    return {};
 }
 std::string
@@ -317,18 +317,17 @@ QPDFFormFieldObjectHelper::setV(QPDFObjectHandle value, bool need_appearances)
                 setCheckBoxValue((name != "/Off"));
             }
             if (!okay) {
-                oh().warnIfPossible(
-                    "ignoring attempt to set a checkbox field to a value whose type is not name");
+                warn("ignoring attempt to set a checkbox field to a value whose type is not name");
             }
         } else if (isRadioButton()) {
             if (value.isName()) {
                 setRadioButtonValue(value);
             } else {
-                oh().warnIfPossible(
+                warn(
                     "ignoring attempt to set a radio button field to an object that is not a name");
             }
         } else if (isPushbutton()) {
-            oh().warnIfPossible("ignoring attempt set the value of a pushbutton field");
+            warn("ignoring attempt set the value of a pushbutton field");
         }
         return;
     }
@@ -375,7 +374,7 @@ QPDFFormFieldObjectHelper::setRadioButtonValue(QPDFObjectHandle name)
     QPDFObjectHandle kids = oh().getKey("/Kids");
     if (!(isRadioButton() && parent.isNull() && kids.isArray())) {
-        oh().warnIfPossible("don't know how to set the value of this field as a radio button");
+        warn("don't know how to set the value of this field as a radio button");
         return;
     }
     setFieldAttribute("/V", name);
@@ -397,7 +396,7 @@ QPDFFormFieldObjectHelper::setRadioButtonValue(QPDFObjectHandle name)
         }
         if (!annot) {
             QTC::TC("qpdf", "QPDFObjectHandle broken radio button");
-            oh().warnIfPossible("unable to set the value of this radio button");
+            warn("unable to set the value of this radio button");
             continue;
         }
         if (AP.isDictionary() && AP.getKey("/N").isDictionary() &&
@@ -453,7 +452,7 @@ QPDFFormFieldObjectHelper::setCheckBoxValue(bool value)
     setFieldAttribute("/V", name);
     if (!annot) {
         QTC::TC("qpdf", "QPDFObjectHandle broken checkbox");
-        oh().warnIfPossible("unable to set the value of this checkbox");
+        warn("unable to set the value of this checkbox");
         return;
     }
     QTC::TC("qpdf", "QPDFFormFieldObjectHelper set checkbox AS");
@@ -770,18 +769,17 @@ QPDFFormFieldObjectHelper::generateTextAppearance(QPDFAnnotationObjectHelper&amp; ao
         AP.replaceKey("/N", AS);
     }
     if (!AS.isStream()) {
-        aoh.getObjectHandle().warnIfPossible("unable to get normal appearance stream for update");
+        aoh.warn("unable to get normal appearance stream for update");
         return;
     }
     if (AS.getObj().use_count() > 4) {
-        aoh.getObjectHandle().warnIfPossible(
-            "unable to generate text appearance from shared appearance stream for update");
+        aoh.warn("unable to generate text appearance from shared appearance stream for update");
         return;
     }
     QPDFObjectHandle bbox_obj = AS.getDict().getKey("/BBox");
     if (!bbox_obj.isRectangle()) {
-        aoh.getObjectHandle().warnIfPossible("unable to get appearance stream bounding box");
+        aoh.warn("unable to get appearance stream bounding box");
         return;
     }
     QPDFObjectHandle::Rectangle bbox = bbox_obj.getArrayAsRectangle();
@@ -835,20 +835,16 @@ QPDFObjectHandle::getValueAsInt(long long&amp; value) const
 int
 QPDFObjectHandle::getIntValueAsInt() const
 {
-    int result = 0;
     long long v = getIntValue();
     if (v < INT_MIN) {
-        QTC::TC("qpdf", "QPDFObjectHandle int returning INT_MIN");
-        warnIfPossible("requested value of integer is too small; returning INT_MIN");
-        result = INT_MIN;
-    } else if (v > INT_MAX) {
-        QTC::TC("qpdf", "QPDFObjectHandle int returning INT_MAX");
-        warnIfPossible("requested value of integer is too big; returning INT_MAX");
-        result = INT_MAX;
-    } else {
-        result = static_cast<int>(v);
+        warn("requested value of integer is too small; returning INT_MIN");
+        return INT_MIN;
     }
-    return result;
+    if (v > INT_MAX) {
+        warn("requested value of integer is too big; returning INT_MAX");
+        return INT_MAX;
+    }
+    return static_cast<int>(v);
 }
 bool
@@ -866,8 +862,7 @@ QPDFObjectHandle::getUIntValue() const
 {
     long long v = getIntValue();
     if (v < 0) {
-        QTC::TC("qpdf", "QPDFObjectHandle uint returning 0");
-        warnIfPossible("unsigned value request for negative number; returning 0");
+        warn("unsigned value request for negative number; returning 0");
         return 0;
     } else {
         return static_cast<unsigned long long>(v);
@@ -889,16 +884,14 @@ QPDFObjectHandle::getUIntValueAsUInt() const
 {
     long long v = getIntValue();
     if (v < 0) {
-        QTC::TC("qpdf", "QPDFObjectHandle uint uint returning 0");
-        warnIfPossible("unsigned integer value request for negative number; returning 0");
+        warn("unsigned integer value request for negative number; returning 0");
         return 0;
-    } else if (v > UINT_MAX) {
-        QTC::TC("qpdf", "QPDFObjectHandle uint returning UINT_MAX");
-        warnIfPossible("requested value of unsigned integer is too big; returning UINT_MAX");
+    }
+    if (v > UINT_MAX) {
+        warn("requested value of unsigned integer is too big; returning UINT_MAX");
         return UINT_MAX;
-    } else {
-        return static_cast<unsigned int>(v);
     }
+    return static_cast<unsigned int>(v);
 }
 bool
@@ -59,7 +59,7 @@ QPDFPageDocumentHelper::flattenAnnotations(int required_flags, int forbidden_fla
     if (afdh.getNeedAppearances()) {
         qpdf.getRoot()
             .getKey("/AcroForm")
-            .warnIfPossible(
+            .warn(
                 "document does not have updated appearance streams, so form fields "
                 "will not be flattened");
     }
@@ -12,6 +12,8 @@
 #include <qpdf/QUtil.hh>
 #include <qpdf/ResourceFinder.hh>
+using namespace std::literals;
+
 namespace
 {
     class ContentProvider: public QPDFObjectHandle::StreamDataProvider
@@ -118,7 +120,7 @@ InlineImageTracker::convertIIDict(QPDFObjectHandle odict)
                         QTC::TC("qpdf", "QPDFPageObjectHelper colorspace lookup");
                         value = colorspace.getKey(name);
                     } else {
-                        resources.warnIfPossible("unable to resolve colorspace " + name);
+                        resources.warn("unable to resolve colorspace " + name);
                     }
                     name.clear();
                 }
@@ -414,8 +416,8 @@ QPDFPageObjectHelper::externalizeInlineImages(size_t min_size, bool shallow)
             filterContents(&iit, &b);
             filtered = true;
         } catch (std::exception& e) {
-            oh().warnIfPossible(
-                std::string("Unable to filter content stream: ") + e.what() +
+            warn(
+                "Unable to filter content stream: "s + e.what() +
                 "; not attempting to externalize inline images from this stream");
         }
         if (filtered && iit.any_images) {
@@ -555,15 +557,15 @@ QPDFPageObjectHelper::removeUnreferencedResourcesHelper(
         ph.parseContents(&rf);
         size_t after_nw = (q ? q->numWarnings() : 0);
         if (after_nw > before_nw) {
-            ph.oh().warnIfPossible(
+            ph.warn(
                 "Bad token found while scanning content stream; "
                 "not attempting to remove unreferenced objects from this object");
             return false;
         }
     } catch (std::exception& e) {
         QTC::TC("qpdf", "QPDFPageObjectHelper bad token finding names");
-        ph.oh().warnIfPossible(
-            std::string("Unable to parse content stream: ") + e.what() +
+        ph.warn(
+            "Unable to parse content stream: "s + e.what() +
             "; not attempting to remove unreferenced objects from this object");
         return false;
     }
@@ -719,8 +721,7 @@ QPDFPageObjectHelper::getFormXObjectForPage(bool handle_transformations)
     newdict.replaceKey("/Group", getAttribute("/Group", false).shallowCopy());
     QPDFObjectHandle bbox = getTrimBox(false).shallowCopy();
     if (!bbox.isRectangle()) {
-        oh().warnIfPossible(
-            "bounding box is invalid; form XObject created from page will not work");
+        warn("bounding box is invalid; form XObject created from page will not work");
     }
     newdict.replaceKey("/BBox", bbox);
     auto provider =
@@ -1396,8 +1396,8 @@ QPDFWriter::willFilterStream(
             }
         } catch (std::runtime_error& e) {
             if (filter && first_attempt) {
-                stream.warnIfPossible("error while getting stream data: "s + e.what());
-                stream.warnIfPossible("qpdf will attempt to write the damaged stream unchanged");
+                stream.warn("error while getting stream data: "s + e.what());
+                stream.warn("qpdf will attempt to write the damaged stream unchanged");
                 filter = false;
                 stream.setFilterOnWrite(false);
                 continue;
@@ -1742,7 +1742,7 @@ QPDFWriter::writeObjectStream(QPDFObjectHandle object)
             if (obj_to_write.isStream()) {
                 // This condition occurred in a fuzz input. Ideally we should block it at parse
                 // time, but it's not clear to me how to construct a case for this.
-                obj_to_write.warnIfPossible("stream found inside object stream; treating as null");
+                obj_to_write.warn("stream found inside object stream; treating as null");
                 obj_to_write = QPDFObjectHandle::newNull();
             }
             writeObject(obj_to_write, count);
@@ -57,7 +57,7 @@ QPDF::getAllPages()
             // Files have been found in the wild where /Pages in the catalog points to the first
             // page. Try to work around this and similar cases with this heuristic.
             if (!warned) {
-                root.warnIfPossible(
+                root.warn(
                     "document page tree root (root -> /Pages) doesn't point"
                     " to the root of the page tree; attempting to correct");
                 warned = true;
@@ -109,7 +109,7 @@ QPDF::getAllPagesInternal(
         // During fuzzing files were encountered where the root object appeared in the pages tree.
         // Unconditionally setting the /Type to /Pages could cause problems, but trying to
         // accommodate the possibility may be excessive.
-        cur_node.warnIfPossible("/Type key should be /Pages but is not; overriding");
+        cur_node.warn("/Type key should be /Pages but is not; overriding");
         cur_node.replaceKey("/Type", "/Pages"_qpdf);
     }
     if (!media_box) {
@@ -134,13 +134,13 @@ QPDF::getAllPagesInternal(
         int errors = 0;
         if (!kid.isDictionary()) {
-            kid.warnIfPossible("Pages tree includes non-dictionary object; ignoring");
+            kid.warn("Pages tree includes non-dictionary object; ignoring");
             m->invalid_page_found = true;
             continue;
         }
         if (!kid.isIndirect()) {
             QTC::TC("qpdf", "QPDF handle direct page object");
-            cur_node.warnIfPossible(
+            cur_node.warn(
                 "kid " + std::to_string(i) + " (from 0) is direct; converting to indirect");
             kid = makeIndirectObject(kid);
             ++errors;
@@ -150,7 +150,7 @@ QPDF::getAllPagesInternal(
         } else {
             if (!media_box && !kid.getKey("/MediaBox").isRectangle()) {
                 QTC::TC("qpdf", "QPDF missing mediabox");
-                kid.warnIfPossible(
+                kid.warn(
                     "kid " + std::to_string(i) +
                     " (from 0) MediaBox is undefined; setting to letter / ANSI A");
                 kid.replaceKey(
@@ -167,7 +167,7 @@ QPDF::getAllPagesInternal(
                 // QPDFPageObjectHelper.
                 QTC::TC("qpdf", "QPDF resolve duplicated page object");
                 if (!m->reconstructed_xref) {
-                    cur_node.warnIfPossible(
+                    cur_node.warn(
                         "kid " + std::to_string(i) +
                         " (from 0) appears more than once in the pages tree;"
                         " creating a new page object as a copy");
@@ -176,7 +176,7 @@ QPDF::getAllPagesInternal(
                     kid = makeIndirectObject(QPDFObjectHandle(kid).shallowCopy());
                     seen.add(kid);
                 } else {
-                    cur_node.warnIfPossible(
+                    cur_node.warn(
                         "kid " + std::to_string(i) +
                         " (from 0) appears more than once in the pages tree; ignoring duplicate");
                     m->invalid_page_found = true;
@@ -189,12 +189,12 @@ QPDF::getAllPagesInternal(
                 }
             }
             if (!kid.isDictionaryOfType("/Page")) {
-                kid.warnIfPossible("/Type key should be /Page but is not; overriding");
+                kid.warn("/Type key should be /Page but is not; overriding");
                 kid.replaceKey("/Type", "/Page"_qpdf);
                 ++errors;
             }
             if (m->reconstructed_xref && errors > 2) {
-                cur_node.warnIfPossible(
+                cur_node.warn(
                     "kid " + std::to_string(i) + " (from 0) has too many errors; ignoring page");
                 m->invalid_page_found = true;
                 kid = QPDFObjectHandle::newNull();
@@ -424,11 +424,6 @@ QPDFPageObjectHelper keep inline image 0
 QPDFJob image optimize colorspace 0
 QPDFJob image optimize bits per component 0
 QPDFWriter remove empty DecodeParms 0
-QPDFObjectHandle uint returning 0 0
-QPDFObjectHandle int returning INT_MIN 0
-QPDFObjectHandle int returning INT_MAX 0
-QPDFObjectHandle uint returning UINT_MAX 0
-QPDFObjectHandle uint uint returning 0 0
 QPDF xref skipped space 0
 QPDF eof skipping spaces before xref 1
 QPDF_encryption user matches owner V < 5 0
@@ -592,7 +587,7 @@ QPDFAcroFormDocumentHelper replaced DA token 0
 QPDFAcroFormDocumentHelper ap conflict 0
 QPDFAcroFormDocumentHelper ap rename 0
 QPDFAcroFormDocumentHelper /DA parse error 0
-QPDFAcroFormDocumentHelper AP parse error 0
+QPDFAcroFormDocumentHelper AP parse error 1
 QPDFJob copy fields not this file 0
 QPDFJob copy fields non-first from orig 0
 QPDF resolve duplicated page in insert 0