Update documentation for PointerHolder transition

Jay Berkenbilt
1 parent ef2b84c6
Showing 3 changed files with 120 additions and 222 deletions
README-maintainer
TODO
manual/design.rst
@@ -160,11 +160,16 @@ CODING RULES
   it seems also for classes that are intended to be subclassed across
   the shared library boundary.
-* Put private member variables in PointerHolder<Members> for all
+* Put private member variables in std::shared_ptr<Members> for all
   public classes. Remember to use QPDF_DLL on ~Members(). Exception:
-  indirection through PointerHolder<Members> is expensive, so don't do
-  it for classes that are copied a lot, like QPDFObjectHandle and
-  QPDFObject.
+  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.
 * Traversal of objects is expensive. It's worth adding some complexity
   to avoid needless traversals of objects.
@@ -5,8 +5,7 @@ Next
 * At next release, hide release-qpdf-10.6.3.0cmake* versions at readthedocs
 In order:
-* cmake
-* PointerHolder -> shared_ptr
+* cmake -- remaining steps
 * ABI including --json default is latest
 * json v2
@@ -29,9 +28,6 @@ Misc
   --show-encryption could potentially retry with this option if the
   first time doesn't work. Then, with the file open, we can read the
   encryption dictionary normally.
-* Go through README-maintainer "CODING RULES" and update --
-  PointerHolder and other changes will make some of the rules
-  obsolete.
 * Have a warn in QPDF that passes its variable arguments onto QPDFExc
   so you don't have to do warn(QPDFExc(...))
@@ -57,6 +53,7 @@ cmake
     QPDF_DLL are public because QPDF_DLL_LOCAL makes the other things
     private. See https://gcc.gnu.org/wiki/Visibility. Make sure this
     is documented.
+  * Update "CODING RULES" in "README-maintainer" - search for QPDF_DLL
 * Nice to have:
   * Split qpdf.test into multiple tests
   * Rework tests so that nothing is written into the source directory.
@@ -500,103 +497,6 @@ Other notes:
   way that works for the qpdf/qpdf repository as well since they are
   very similar.
-PointerHolder to std::shared_ptr
-================================
-
-To perform update:
-
-Cherry-pick pointerholder branch commit
-
-Upgrade just the library. This is not necessary, but it's an added
-check that the compatibility code works since it will show that tests,
-examples, and CLI will work properly with the upgraded APIs, which
-provides some assurance that other people will have a smooth time with
-their code.
-
-patrepl s/PointerHolder/std::shared_ptr/g {include,libqpdf}/qpdf/*.hh
-patrepl s/PointerHolder/std::shared_ptr/g libqpdf/*.cc
-patrepl s/make_pointer_holder/std::make_shared/g libqpdf/*.cc
-patrepl s/make_array_pointer_holder/QUtil::make_shared_array/g libqpdf/*.cc
-patrepl s,qpdf/std::shared_ptr,qpdf/PointerHolder, **/*.cc **/*.hh
-git restore include/qpdf/PointerHolder.hh
-cleanpatch
-
-Increase to POINTERHOLDER_TRANSITION=3
-
-make build_libqpdf -- no errors
-
-Drop back to POINTERHOLDER_TRANSITION=2
-
-make check -- everything passes
-
-Then upgrade everything else. It would work to just start here.
-
-Increase to POINTERHOLDER_TRANSITION=3
-
-patrepl s/PointerHolder/std::shared_ptr/g **/*.cc **/*.hh
-patrepl s/make_pointer_holder/std::make_shared/g **/*.cc
-patrepl s/make_array_pointer_holder/QUtil::make_shared_array/g **/*.cc
-patrepl s,qpdf/std::shared_ptr,qpdf/PointerHolder, **/*.cc **/*.hh
-git restore include/qpdf/PointerHolder.hh
-git restore libtests/pointer_holder.cc
-cleanpatch
-
-Remove all references to PointerHolder.hh from everything except
-public headers and pointer_holder.cc.
-
-make check -- everything passes
-
-Increase to POINTERHOLDER_TRANSITION=4
-
-Do a clean build and make check -- everything passes
-
-Final steps:
-
-* Change to POINTERHOLDER_TRANSITION=4
-* Check code formatting
-* std::shared_ptr<Members> m can be replaced with
-  std::shared_ptr<Members> m_ph and Members* m if performance is critical
-* Could try Members indirection with Members* for QPDFObjectHandle
-
-When done:
-
-* Update the smart-pointers section of the manual in design.rst
-* Update comments in PointerHolder.hh
-
-PointerHolder in public API:
-
-  PointerHolder<Buffer> Pl_Buffer::getBufferSharedPointer();
-  PointerHolder<Buffer> QPDFWriter::getBufferSharedPointer();
-  QUtil::read_file_into_memory(
-    char const*, PointerHolder<char>&, unsigned long&)
-  QPDFObjectHandle::addContentTokenFilter(
-    PointerHolder<QPDFObjectHandle::TokenFilter>)
-  QPDFObjectHandle::addTokenFilter(
-    PointerHolder<QPDFObjectHandle::TokenFilter>)
-  QPDFObjectHandle::newStream(
-    QPDF*, PointerHolder<Buffer>)
-  QPDFObjectHandle::parse(
-    PointerHolder<InputSource>, std::string const&,
-    QPDFTokenizer&, bool&, QPDFObjectHandle::StringDecrypter*, QPDF*)
-  QPDFObjectHandle::replaceStreamData(
-    PointerHolder<Buffer>, QPDFObjectHandle const&,
-    QPDFObjectHandle const&)
-  QPDFObjectHandle::replaceStreamData(
-    PointerHolder<QPDFObjectHandle::StreamDataProvider>,
-    QPDFObjectHandle const&, QPDFObjectHandle const&)
-  QPDFTokenizer::expectInlineImage(
-    PointerHolder<InputSource>)
-  QPDFTokenizer::readToken(
-    PointerHolder<InputSource>, std::string const&,
-    bool, unsigned long)
-  QPDF::processInputSource(
-    PointerHolder<InputSource>, char const*)
-  QPDFWriter::registerProgressReporter(
-    PointerHolder<QPDFWriter::ProgressReporter>)
-  QPDFEFStreamObjectHelper::createEFStream(
-    QPDF&, PointerHolder<Buffer>)
-  QPDFPageObjectHelper::addContentTokenFilter(
-    PointerHolder<QPDFObjectHandle::TokenFilter>)
 ABI Changes
 ===========
@@ -53,13 +53,11 @@ or not if this information is needed. All access to objects deals with
 this transparently. All memory management details are also handled by
 the library.
-The ``PointerHolder`` object is used internally by the library to deal
-with memory management. This is basically a smart pointer object very
-similar in spirit to C++-11's ``std::shared_ptr`` object, but predating
-it by several years. This library also makes use of a technique for
-giving fine-grained access to methods in one class to other classes by
-using public subclasses with friends and only private members that in
-turn call private methods of the containing class. See
+Memory is managed mostly with ``std::shared_ptr`` object to minimize
+explicit memory handling. This library also makes use of a technique
+for giving fine-grained access to methods in one class to other
+classes by using public subclasses with friends and only private
+members that in turn call private methods of the containing class. See
 ``QPDFObjectHandle::Factory`` as an example.
 The top-level qpdf class is ``QPDF``. A ``QPDF`` object represents a PDF
@@ -68,13 +66,14 @@ files.
 The primary class for interacting with PDF objects is
 ``QPDFObjectHandle``. Instances of this class can be passed around by
-value, copied, stored in containers, etc. with very low overhead.
-Instances of ``QPDFObjectHandle`` created by reading from a file will
-always contain a reference back to the ``QPDF`` object from which they
-were created. A ``QPDFObjectHandle`` may be direct or indirect. If
-indirect, the ``QPDFObject`` the ``PointerHolder`` initially points to
-is a null pointer. In this case, the first attempt to access the
-underlying ``QPDFObject`` will result in the ``QPDFObject`` being
+value, copied, stored in containers, etc. with very low overhead. The
+``QPDFObjectHandle`` object contains an internal shared pointer to an
+underyling ``QPDFObject``. Instances of ``QPDFObjectHandle`` created
+by reading from a file will always contain a reference back to the
+``QPDF`` object from which they were created. A ``QPDFObjectHandle``
+may be direct or indirect. If indirect, the ``QPDFObject`` shared
+pointer is initially null. In this case, the first attempt to access
+the underlying ``QPDFObject`` will result in the ``QPDFObject`` being
 resolved via a call to the referenced ``QPDF`` instance. This makes it
 essentially impossible to make coding errors in which certain things
 will work for some PDF files and not for others based on which objects
@@ -248,17 +247,18 @@ During this process, the &quot;``R``&quot; keyword is recognized and an indirect
 The ``QPDF::resolve()`` method, which is used to resolve an indirect
 object, may be invoked from the ``QPDFObjectHandle`` class. It first
-checks a cache to see whether this object has already been read. If not,
-it reads the object from the PDF file and caches it. It the returns the
-resulting ``QPDFObjectHandle``. The calling object handle then replaces
-its ``PointerHolder<QDFObject>`` with the one from the newly returned
-``QPDFObjectHandle``. In this way, only a single copy of any direct
-object need exist and clients can access objects transparently without
-knowing or caring whether they are direct or indirect objects.
-Additionally, no object is ever read from the file more than once. That
-means that only the portions of the PDF file that are actually needed
-are ever read from the input file, thus allowing the qpdf package to
-take advantage of this important design goal of PDF files.
+checks a cache to see whether this object has already been read. If
+not, it reads the object from the PDF file and caches it. It the
+returns the resulting ``QPDFObjectHandle``. The calling object handle
+then replaces its ``std::shared_ptr<QDFObject>`` with the one from the
+newly returned ``QPDFObjectHandle``. In this way, only a single copy
+of any direct object need exist and clients can access objects
+transparently without knowing or caring whether they are direct or
+indirect objects. Additionally, no object is ever read from the file
+more than once. That means that only the portions of the PDF file that
+are actually needed are ever read from the input file, thus allowing
+the qpdf package to take advantage of this important design goal of
+PDF files.
 If the requested object is inside of an object stream, the object stream
 itself is first read into memory. Then the tokenizer reads objects from
@@ -762,40 +762,47 @@ Smart Pointers
 --------------
 This section describes changes to the use of smart pointers that were
-made in qpdf 10.6.0 as well as some planned for 11.0.0.
+made in qpdf 10.6.0 and 11.0.0.
-Starting in qpdf 11, ``PointerHolder`` will be replaced with
+In qpdf 11.0.0, ``PointerHolder`` was replaced with
 ``std::shared_ptr`` in qpdf's public API. A backward-compatible
-``PointerHolder`` class will be provided that should make it possible
-for most code to remain unchanged. ``PointerHolder`` may eventually be
+``PointerHolder`` class has been provided that makes it possible for
+most code to remain unchanged. ``PointerHolder`` may eventually be
 removed from qpdf entirely, but this will not happen for a while to
 make it easier for people who need to support multiple versions of
 qpdf.
-The ``POINTERHOLDER_TRANSITION`` preprocessor symbol has been
-introduced to help people transition from ``PointerHolder`` to
-``std::shared_ptr``. After qpdf 11 is released, to prepare for a
-future qpdf without ``PointerHolder`` and to let them know that it is
-no longer needed, a warning will be issued if
-``<qpdf/PointerHolder.hh>`` is included, though it will be possible to
-suppress the warning by defining ``POINTERHOLDER_TRANSITION``. In
-10.6.0, there are some steps you can perform to prepare, but no action
-is required.
+In 10.6.0, some enhancements were made to ``PointerHolder`` to ease
+the transition. These intermediate steps are relevant only for
+versions 10.6.0 through 10.6.3 but can still help with incremental
+modification of code.
-The remainder of this section describes how to prepare if you want to
-eliminate ``PointerHolder`` from your code or what to do if you want
-to stick with the old interfaces.
+The ``POINTERHOLDER_TRANSITION`` preprocessor symbol was introduced in
+qpdf 10.6.0 to help people transition from ``PointerHolder`` to
+``std::shared_ptr``. If you don't define this, you will get a compiler
+warning. Defining it to any value will suppress the warning. An
+explanation appears below of the different possible values for this
+symbol and what they mean.
-Changes in 10.6.0
-~~~~~~~~~~~~~~~~~
+Starting in qpdf 11.0.0, including ``<qpdf/PointerHolder.hh>`` defines
+the symbol ``POINTERHOLDER_IS_SHARED_POINTER``. This can be used with
+conditional compilation to make it possible to support different
+versions of qpdf.
-In qpdf 10.6.0, the following changes have been made to
-``PointerHolder`` to make its behavior closer to that of
-``std::shared_ptr``:
+The rest of this section provides the details.
-- ``get()`` has been added as an alternative to ``getPointer()``
+Transitional Enhancements to PointerHolder
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-- ``use_count()`` has been added as an alternative to ``getRefcount()``
+In qpdf 10.6.0, some changes were to ``PointerHolder`` to make it
+easier to prepare for the transition to ``std::shared_ptr``. These
+enhancements also make it easier to incrementally upgrade your code.
+The following changes were made to ``PointerHolder`` to make its
+behavior closer to that of ``std::shared_ptr``:
+
+- ``get()`` was added as an alternative to ``getPointer()``
+
+- ``use_count()`` was added as an alternative to ``getRefcount()``
 - A new global helper function ``make_pointer_holder`` behaves
   similarly to ``std::make_shared``, so you can use
@@ -807,7 +814,7 @@ In qpdf 10.6.0, the following changes have been made to
   counterpart to the newly added ``QUtil::make_shared_array`` method,
   which does the same thing with a ``std::shared_ptr``.
-``PointerHolder`` has had a long-standing bug: a ``const
+``PointerHolder`` had a long-standing bug: a ``const
 PointerHolder<T>`` would only provide a ``T const*`` with its
 ``getPointer`` method. This is incorrect and is not how standard
 library C++ smart pointers or regular pointers behave. The correct
@@ -873,16 +880,23 @@ preprocessor symbol or other C++ coding techniques.
   pointer. It also has the seldom-used ``getRefcount()`` method to get
   the reference count. ``std::shared_ptr<T>`` has ``get()`` and
   ``use_count()``. In qpdf 10.6, ``PointerHolder<T>`` also has
-  would not be an issue unless you did this in your own code.
+  ``get()`` and ``use_count()``.
 Addressing the Differences
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
-If you need to support versions of qpdf prior to qpdf 10.6, you don't
-*need* to take any action at this time, but it is recommended that you
-at least address the implicit constructor issue since this can be done
-without breaking backward compatibility. (Explicit construction of
-``PointerHolder<T>`` is and always has been allowed.)
+If you are not ready to take action yet, you can ``#define
+POINTERHOLDER_TRANSITION 0`` before including any qpdf header file or
+add the definition of that symbol to your build. This will provide the
+backward-compatible ``PointerHolder`` API without any deprecation
+warnings. This should be a temporary measure as ``PointerHolder`` may
+disappear in the future. If you need to be able to support newer and
+older versions of qpdf, there are other options, explained below.
+
+Note that, even with ``0``, you should rebuild and test your code.
+There may be compiler errors if you have containers of
+``PointerHolder``, but most code should compile without any changes.
+There are no uses of containers of ``PointerHolder`` in qpdf's API.
 There are two significant things you can do to minimize the impact of
 switching from ``PointerHolder`` to ``std::shared_ptr``:
@@ -908,31 +922,35 @@ without consulting this manual.
      - meaning
    - - undefined
-     - Same as ``0``, but starting with qpdf 11.0, issues a warning
+     - Same as ``0`` but issues a warning
    - - ``0``
      - Provide a backward compatible ``PointerHolder`` and suppress
-       all deprecation warnings
+       all deprecation warnings; supports all prior qpdf versions
    - - ``1``
-     - Make the ``PointerHolder<T>(T*)`` constructor explicit
+     - Make the ``PointerHolder<T>(T*)`` constructor explicit;
+       resulting code supports all prior qpdf versions
    - - ``2``
-     - Deprecate ``getPointer()`` and ``getRefcount()``
+     - Deprecate ``getPointer()`` and ``getRefcount()``; requires
+       qpdf 10.6.0 or later.
    - - ``3``
-     - Starting with qpdf 11.0, deprecate all uses of ``PointerHolder``
+     - Deprecate all uses of ``PointerHolder``; requires qpdf 11.0.0
+       or later
    - - ``4``
-     - Starting with qpdf 11.0, disable all functionality from
-       ``qpdf/PointerHolder.hh`` so that ``#include``-ing it has no
-       effect.
+     - Disable all functionality from ``qpdf/PointerHolder.hh`` so
+       that ``#include``-ing it has no effect other than defining
+       ``POINTERHOLDER_IS_SHARED_POINTER``; requires qpdf 11.0.0 or
+       later.
 Based on the above, here is a procedure for preparing your code. This
 is the procedure that was used for the qpdf code itself.
-If you need to support versions of qpdf prior to 10.6, you can still
-do these steps:
+You can do these steps without breaking support for qpdf versions
+before 10.6.0:
 - Find all occurrences of ``PointerHolder`` in the code. See whether
   any of them can just be outright replaced with ``std::shared_ptr``
@@ -974,8 +992,9 @@ do these steps:
        auto p = std::unique_ptr<X[]>(new X[n]);
 - If a ``PointerHolder<T>`` can't be replaced with a standard library
-  smart pointer, perhaps it can be declared using ``auto`` or
-  ``decltype`` so that, when the qpdf API changes, your code will just
+  smart pointer because it is used with an older qpdf API call,
+  perhaps it can be declared using ``auto`` or ``decltype`` so that,
+  when building with a newer qpdf API changes, your code will just
   need to be recompiled.
 - ``#define POINTERHOLDER_TRANSITION 1`` to enable deprecation
@@ -1000,55 +1019,18 @@ do these steps:
     Other examples appear above.
 If you need to support older versions of qpdf than 10.6, this is as
-far as you can go until qpdf 11 comes out.
-
-If you only need to support the latest version of qpdf, proceed as
-follows:
-
-- ``#define POINTERHOLDER_TRANSITION 2`` to enable deprecation of
-  ``getPointer()`` and ``getRefcount()``
-
-- Replace ``getPointer()`` with ``get()`` and ``getRefcount()`` with
-  ``use_count()``. These methods were not present prior to 10.6.0.
+far as you can go without conditional compilation.
-When you have gotten your code to compile cleanly with
-``POINTERHOLDER_TRANSITION=2``, you are well on your way to being
-ready for eliminating ``PointerHolder`` entirely after qpdf 11 is
-released.
-
-After qpdf 11 is out
-~~~~~~~~~~~~~~~~~~~~
-
-In the 10.6 manual, this section represents a plan and is subject to
-change. However, it has been tested in practice using a version of the
-qpdf 11 ``PointerHolder`` on a branch, so it is likely to be accurate.
-In the meantime, think of this as a preview.
-
-First, make sure you have done the steps in the 10.6 section. (Note:
-once qpdf 11 comes out, the goal is to not have to migrate to 10.6
-first, so it is likely that these sections will be combined.)
-
-If you are explicitly choosing to stick with the backward compatible
-``PointerHolder`` for now, you should define
-``POINTERHOLDER_TRANSITION`` to ``0`` to suppress the warning from
-including ``qpdf/PointerHolder.hh``. Be aware that you may eventually
-have to deal with the transition, though the intention is to leave the
-compatibility layer in place for a while. You should rebuild and test
-your code. There may be compiler errors if you have containers of
-``PointerHolder``, but most code should compile without any changes.
-Even if you have errors, use of ``auto`` or ``decltype`` may enable
-you to write code that works with the old and new API without having
-to use conditional compilation. The
-``POINTERHOLDER_IS_SHARED_POINTER`` is defined in qpdf 11 if you
-``#include <qpdf/PointerHolder.hh>``.
-
-If you want to support older versions of qpdf and still transition so
-that the backward-compatible ``PointerHolder`` is not in use, you can
-separate old code and new code by testing with the
+Starting in qpdf 11.0.0, including ``<qpdf/PointerHolder.hh>`` defines
+the symbol ``POINTERHOLDER_IS_SHARED_POINTER``. If you want to support
+older versions of qpdf and still transition so that the
+backward-compatible ``PointerHolder`` is not in use, you can separate
+old code and new code by testing with the
 ``POINTERHOLDER_IS_SHARED_POINTER`` preprocessor symbol, as in
 .. code-block:: c++
+   #include <qpdf/PointerHolder.hh>
    #ifdef POINTERHOLDER_IS_SHARED_POINTER
    std::shared_ptr<X> x;
    #else
@@ -1060,6 +1042,7 @@ or
 .. code-block:: c++
+   #include <qpdf/PointerHolder.hh>
    #ifdef POINTERHOLDER_IS_SHARED_POINTER
    auto x_p = std::make_shared<X>();
    X* x = x_p.get();
@@ -1074,13 +1057,23 @@ If you don&#39;t need to support older versions of qpdf, you can proceed
 with these steps without protecting changes with the preprocessor
 symbol. Here are the remaining changes.
-- Make sure you have a clean build with ``POINTERHOLDER_TRANSITION``
-  set to ``2``. This means that you are using ``PointerHolder`` in a
-  manner that is API-compatible with ``std::shared_ptr`` in all cases
-  except for array pointers.
+- ``#define POINTERHOLDER_TRANSITION 2`` to enable deprecation of
+  ``getPointer()`` and ``getRefcount()``
+
+- Replace ``getPointer()`` with ``get()`` and ``getRefcount()`` with
+  ``use_count()``. These methods were not present prior to 10.6.0.
+
+When you have gotten your code to compile cleanly with
+``POINTERHOLDER_TRANSITION=2``, you are well on your way to being
+ready for eliminating ``PointerHolder`` entirely. The code at this
+point will not work with any qpdf version prior to 10.6.0.
+
+To support qpdf 11.0.0 and newer and remove ``PointerHolder`` from
+your code, continue with the following steps:
 - Replace all occurrences of ``PointerHolder`` with
-  ``std::shared_ptr`` except in ``#include <qpdf/PointerHolder.hh>``
+  ``std::shared_ptr`` except in the literal statement ``#include
+  <qpdf/PointerHolder.hh>``
 - Replace all occurrences of ``make_pointer_holder`` with
   ``std::make_shared``