diff options
-rw-r--r-- | README-maintainer.md | 30 | ||||
-rw-r--r-- | libqpdf/JSONHandler.cc | 65 | ||||
-rw-r--r-- | libqpdf/QPDF.cc | 3 | ||||
-rw-r--r-- | libqpdf/qpdf/JSONHandler.hh | 51 |
4 files changed, 77 insertions, 72 deletions
diff --git a/README-maintainer.md b/README-maintainer.md index 4f11fdcf..254c248f 100644 --- a/README-maintainer.md +++ b/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. diff --git a/libqpdf/JSONHandler.cc b/libqpdf/JSONHandler.cc index b5c7c35d..4a69fd60 100644 --- a/libqpdf/JSONHandler.cc +++ b/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"); } diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc index 4df4c264..396dfe8f 100644 --- a/libqpdf/QPDF.cc +++ b/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")); diff --git a/libqpdf/qpdf/JSONHandler.hh b/libqpdf/qpdf/JSONHandler.hh index 8fdcaee6..653924f6 100644 --- a/libqpdf/qpdf/JSONHandler.hh +++ b/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 - { - friend class JSONHandler; - - public: - ~Members() = default; - - private: - Members() = default; - Members(Members const&) = delete; - - Handlers h; - }; - std::shared_ptr<Members> m; + + class Members; + + std::unique_ptr<Members> m; }; #endif // JSONHANDLER_HH |