Merge pull request #985 from m-holger/members

Change JSONHandler::m to std::unique_ptr and declare Members in implementation file

README-maintainer.md

@@ -235,16 +235,26 @@ Building docs from pull requests is also enabled.
     // README-maintainer
     ```
 
-* Put private member variables in std::shared_ptr<Members> for all
-  public classes. Remember to use QPDF_DLL on ~Members(). Exception:
-  indirection through std::shared_ptr<Members> is expensive, so don't
-  do it for classes that are copied a lot, like QPDFObjectHandle and
-  QPDFObject. It may be possible to declare
-  std::shared_ptr<Members> m_ph;
-  Member* m;
-  with m = m_ph.get(), and then indirect through m in
-  performance-critical settings, though in 2022, std::shared_ptr is
-  sufficiently performant that this may not be worth it.
+* Put private member variables in std::unique_ptr<Members> for all
+  public classes. Forward declare Members in the header file and define
+  Members in the implementation file. One of the major benefits of
+  defining Members in the implementation file is that it makes it easier
+  to use private classes as data members and simplifies the include order.
+  Remember that Members must be fully defined before the destructor of the
+  main class. For an example of this pattern see class JSONHandler.
+
+  Exception: indirection through std::unique_ptr<Members> incurs an overhead,
+  so don't do it for:
+  * (especially private) classes that are copied a lot, like QPDFObjectHandle
+    and QPDFObject.
+  * classes that are a shared pointer to another class, such as QPDFObjectHandle
+    or JSON.
+
+  For exported classes that do not use the member pattern for performance
+  reasons it is worth considering adding a std::unique_ptr to an empty Members
+  class initialized to nullptr to give the flexibility to add data members
+  without breaking the ABI.
+
 
 * Traversal of objects is expensive. It's worth adding some complexity
   to avoid needless traversals of objects.

libqpdf/JSONHandler.cc

@@ -4,11 +4,48 @@
 #include <qpdf/QTC.hh>
 #include <qpdf/QUtil.hh>
 
+struct Handlers
+{
+    Handlers() = default;
+
+    JSONHandler::json_handler_t any_handler{nullptr};
+    JSONHandler::void_handler_t null_handler{nullptr};
+    JSONHandler::string_handler_t string_handler{nullptr};
+    JSONHandler::string_handler_t number_handler{nullptr};
+    JSONHandler::bool_handler_t bool_handler{nullptr};
+    JSONHandler::json_handler_t dict_start_handler{nullptr};
+    JSONHandler::void_handler_t dict_end_handler{nullptr};
+    JSONHandler::json_handler_t array_start_handler{nullptr};
+    JSONHandler::void_handler_t array_end_handler{nullptr};
+    JSONHandler::void_handler_t final_handler{nullptr};
+    std::map<std::string, std::shared_ptr<JSONHandler>> dict_handlers;
+    std::shared_ptr<JSONHandler> fallback_dict_handler;
+    std::shared_ptr<JSONHandler> array_item_handler;
+};
+
+class JSONHandler::Members
+{
+    friend class JSONHandler;
+
+  public:
+    ~Members() = default;
+
+  private:
+    Members() = default;
+    Members(Members const&) = delete;
+
+    Handlers h;
+};
+
 JSONHandler::JSONHandler() :
     m(new Members())
 {
 }
 
+JSONHandler::~JSONHandler()
+{
+}
+
 void
 JSONHandler::usage(std::string const& msg)
 {
@@ -80,24 +117,24 @@ JSONHandler::handle(std::string const& path, JSON j)
         m->h.any_handler(path, j);
         return;
     }
-    bool handled = false;
+
     bool bvalue = false;
     std::string s_value;
     if (m->h.null_handler && j.isNull()) {
         m->h.null_handler(path);
-        handled = true;
+        return;
     }
     if (m->h.string_handler && j.getString(s_value)) {
         m->h.string_handler(path, s_value);
-        handled = true;
+        return;
     }
     if (m->h.number_handler && j.getNumber(s_value)) {
         m->h.number_handler(path, s_value);
-        handled = true;
+        return;
     }
     if (m->h.bool_handler && j.getBool(bvalue)) {
         m->h.bool_handler(path, bvalue);
-        handled = true;
+        return;
     }
     if (m->h.dict_start_handler && j.isDictionary()) {
         m->h.dict_start_handler(path, j);
@@ -119,7 +156,7 @@ JSONHandler::handle(std::string const& path, JSON j)
             }
         });
         m->h.dict_end_handler(path);
-        handled = true;
+        return;
     }
     if (m->h.array_start_handler && j.isArray()) {
         m->h.array_start_handler(path, j);
@@ -129,15 +166,13 @@ JSONHandler::handle(std::string const& path, JSON j)
             ++i;
         });
         m->h.array_end_handler(path);
-        handled = true;
+        return;
     }
 
-    if (!handled) {
-        // It would be nice to include information about what type the object was and what types
-        // were allowed, but we're relying on schema validation to make sure input is properly
-        // structured before calling the handlers. It would be different if this code were trying to
-        // be part of a general-purpose JSON package.
-        QTC::TC("libtests", "JSONHandler unhandled value");
-        usage("JSON handler: value at " + path + " is not of expected type");
-    }
+    // It would be nice to include information about what type the object was and what types were
+    // allowed, but we're relying on schema validation to make sure input is properly structured
+    // before calling the handlers. It would be different if this code were trying to be part of a
+    // general-purpose JSON package.
+    QTC::TC("libtests", "JSONHandler unhandled value");
+    usage("JSON handler: value at " + path + " is not of expected type");
 }

libqpdf/QPDF.cc

@@ -1482,7 +1482,8 @@ QPDF::readObjectAtOffset(
 
     // Special case: if offset is 0, just return null.  Some PDF writers, in particular
     // "Mac OS X 10.7.5 Quartz PDFContext", may store deleted objects in the xref table as
-    // "0000000000 00000 n", which is not correct, but it won't hurt anything for to ignore these.
+    // "0000000000 00000 n", which is not correct, but it won't hurt anything for us to ignore
+    // these.
     if (offset == 0) {
         QTC::TC("qpdf", "QPDF bogus 0 offset", 0);
         warn(damagedPDF(0, "object has offset 0"));

libqpdf/qpdf/JSONHandler.hh

@@ -16,7 +16,7 @@ class JSONHandler
   public:
     // A QPDFUsage exception is thrown if there are any errors validating the JSON object.
     JSONHandler();
-    ~JSONHandler() = default;
+    ~JSONHandler();
 
     // Based on the type of handler, expect the object to be of a certain type. QPDFUsage is thrown
     // otherwise. Multiple handlers may be registered, which allows the object to be of various
@@ -53,51 +53,10 @@ class JSONHandler
 
     static void usage(std::string const& msg);
 
-    struct Handlers
-    {
-        Handlers() :
-            any_handler(nullptr),
-            null_handler(nullptr),
-            string_handler(nullptr),
-            number_handler(nullptr),
-            bool_handler(nullptr),
-            dict_start_handler(nullptr),
-            dict_end_handler(nullptr),
-            array_start_handler(nullptr),
-            array_end_handler(nullptr),
-            final_handler(nullptr)
-        {
-        }
 
-        json_handler_t any_handler;
-        void_handler_t null_handler;
-        string_handler_t string_handler;
-        string_handler_t number_handler;
-        bool_handler_t bool_handler;
-        json_handler_t dict_start_handler;
-        void_handler_t dict_end_handler;
-        json_handler_t array_start_handler;
-        void_handler_t array_end_handler;
-        void_handler_t final_handler;
-        std::map<std::string, std::shared_ptr<JSONHandler>> dict_handlers;
-        std::shared_ptr<JSONHandler> fallback_dict_handler;
-        std::shared_ptr<JSONHandler> array_item_handler;
-    };
+    class Members;
 
-    class Members
-    {
-        friend class JSONHandler;
-
-      public:
-        ~Members() = default;
-
-      private:
-        Members() = default;
-        Members(Members const&) = delete;
-
-        Handlers h;
-    };
-    std::shared_ptr<Members> m;
+    std::unique_ptr<Members> m;
 };
 
 #endif // JSONHANDLER_HH