Rewrite PointerHolder as derived from std::shared_ptr

3 files changed, 225 insertions, 161 deletions

diff --git a/include/qpdf/PointerHolder.hh b/include/qpdf/PointerHolder.hh
index fb186778..b51ca7ba 100644
--- a/include/qpdf/PointerHolder.hh
+++ b/include/qpdf/PointerHolder.hh
@@ -39,6 +39,8 @@
 # define POINTERHOLDER_TRANSITION 0
 #endif // !defined(POINTERHOLDER_TRANSITION)
 
+#if POINTERHOLDER_TRANSITION < 4
+
 // *** WHAT IS HAPPENING ***
 
 // In qpdf 11, PointerHolder will be replaced with std::shared_ptr
@@ -140,211 +142,94 @@
 // written code. If you are relying on the incorrect behavior, use
 // PointerHolder<T const> instead.
 
-// OLD DOCUMENTATION
-
-// This class is basically std::shared_ptr but predates that by
-// several years.
-
-// This class expects to be initialized with a dynamically allocated
-// object pointer.  It keeps a reference count and deletes this once
-// the reference count goes to zero.  PointerHolder objects are
-// explicitly safe for use in STL containers.
-
-// It is very important that a client who pulls the pointer out of
-// this holder does not let the holder go out of scope until it is
-// finished with the pointer.  It is also important that exactly one
-// instance of this object ever gets initialized with a given pointer.
-// Otherwise, the pointer will be deleted twice, and before that, some
-// objects will be left with a pointer to a deleted object.  In other
-// words, the only legitimate way for two PointerHolder objects to
-// contain the same pointer is for one to be a copy of the other.
-// Copy and assignment semantics are well-defined and essentially
-// allow you to use PointerHolder as a means to get pass-by-reference
-// semantics in a pass-by-value environment without having to worry
-// about memory management details.
-
-// Comparison (== and <) are defined and operate on the internally
-// stored pointers, not on the data.  This makes it possible to store
-// PointerHolder objects in sorted lists or to find them in STL
-// containers just as one would be able to store pointers.  Comparing
-// the underlying pointers provides a well-defined, if not
-// particularly meaningful, ordering.
-
-#include <cstddef>
+# include <cstddef>
+# include <memory>
 
 template <class T>
-class PointerHolder
+class PointerHolder: public std::shared_ptr<T>
 {
-  private:
-    class Data
-    {
-      public:
-        Data(T* pointer, bool array) :
-            pointer(pointer),
-            array(array),
-            refcount(0)
-        {
-        }
-        ~Data()
-        {
-            if (array) {
-                delete[] this->pointer;
-            } else {
-                delete this->pointer;
-            }
-        }
-        T* pointer;
-        bool array;
-        int refcount;
-
-      private:
-        Data(Data const&) = delete;
-        Data& operator=(Data const&) = delete;
-    };
-
   public:
-#if POINTERHOLDER_TRANSITION >= 1
-    explicit
-#endif // POINTERHOLDER_TRANSITION >= 1
-        PointerHolder(T* pointer = 0)
-    {
-        this->init(new Data(pointer, false));
-    }
-    // Special constructor indicating to free memory with delete []
-    // instead of delete
-    PointerHolder(bool, T* pointer)
-    {
-        this->init(new Data(pointer, true));
-    }
-    PointerHolder(PointerHolder const& rhs)
-    {
-        this->copy(rhs);
-    }
-    PointerHolder&
-    operator=(PointerHolder const& rhs)
-    {
-        if (this != &rhs) {
-            this->destroy();
-            this->copy(rhs);
-        }
-        return *this;
-    }
-    PointerHolder&
-    operator=(decltype(nullptr))
+# if POINTERHOLDER_TRANSITION >= 3
+    [[deprecated("use std::shared_ptr<T> instead")]]
+# endif // POINTERHOLDER_TRANSITION >= 3
+    PointerHolder(std::shared_ptr<T> other) :
+        std::shared_ptr<T>(other)
     {
-        this->operator=(PointerHolder<T>());
-        return *this;
     }
-    ~PointerHolder()
-    {
-        this->destroy();
-    }
-    bool
-    operator==(PointerHolder const& rhs) const
-    {
-        return this->data->pointer == rhs.data->pointer;
-    }
-    bool
-    operator==(decltype(nullptr)) const
+# if POINTERHOLDER_TRANSITION >= 3
+    [[deprecated("use std::shared_ptr<T> instead")]]
+#  if POINTERHOLDER_TRANSITION >= 1
+    explicit
+#  endif // POINTERHOLDER_TRANSITION >= 1
+# endif  // POINTERHOLDER_TRANSITION >= 3
+        PointerHolder(T* pointer = 0) :
+        std::shared_ptr<T>(pointer)
     {
-        return this->data->pointer == nullptr;
     }
-    bool
-    operator<(PointerHolder const& rhs) const
+    // Create a shared pointer to an array
+# if POINTERHOLDER_TRANSITION >= 3
+    [[deprecated("use std::shared_ptr<T> instead")]]
+# endif // POINTERHOLDER_TRANSITION >= 3
+    PointerHolder(bool, T* pointer) :
+        std::shared_ptr<T>(pointer, std::default_delete<T[]>())
     {
-        return this->data->pointer < rhs.data->pointer;
     }
 
-    // get() is for interface compatibility with std::shared_ptr
-    T*
-    get() const
-    {
-        return this->data->pointer;
-    }
+    virtual ~PointerHolder() = default;
 
-    // NOTE: The pointer returned by getPointer turns into a pumpkin
-    // when the last PointerHolder that contains it disappears.
-#if POINTERHOLDER_TRANSITION >= 2
+# if POINTERHOLDER_TRANSITION >= 2
     [[deprecated("use PointerHolder<T>::get() instead of getPointer()")]]
-#endif // POINTERHOLDER_TRANSITION >= 2
+# endif // POINTERHOLDER_TRANSITION >= 2
     T*
     getPointer()
     {
-        return this->data->pointer;
+        return this->get();
     }
-#if POINTERHOLDER_TRANSITION >= 2
+# if POINTERHOLDER_TRANSITION >= 2
     [[deprecated("use PointerHolder<T>::get() instead of getPointer()")]]
-#endif // POINTERHOLDER_TRANSITION >= 2
+# endif // POINTERHOLDER_TRANSITION >= 2
     T const*
     getPointer() const
     {
-        return this->data->pointer;
+        return this->get();
     }
-#if POINTERHOLDER_TRANSITION >= 2
-    [[deprecated("use use_count() instead of getRefcount()")]]
-#endif // POINTERHOLDER_TRANSITION >= 2
+
+# if POINTERHOLDER_TRANSITION >= 2
+    [[deprecated("use PointerHolder<T>::get() instead of getPointer()")]]
+# endif // POINTERHOLDER_TRANSITION >= 2
     int
     getRefcount() const
     {
-        return this->data->refcount;
+        return static_cast<int>(this->use_count());
     }
 
-    // use_count() is for compatibility with std::shared_ptr
-    long
-    use_count()
+    PointerHolder&
+    operator=(decltype(nullptr))
     {
-        return static_cast<long>(this->data->refcount);
+        std::shared_ptr<T>::operator=(nullptr);
+        return *this;
     }
-
     T const&
     operator*() const
     {
-        return *this->data->pointer;
+        return *(this->get());
     }
     T&
     operator*()
     {
-        return *this->data->pointer;
+        return *(this->get());
     }
 
     T const*
     operator->() const
     {
-        return this->data->pointer;
+        return this->get();
     }
     T*
     operator->()
     {
-        return this->data->pointer;
+        return this->get();
     }
-
-  private:
-    void
-    init(Data* data)
-    {
-        this->data = data;
-        ++this->data->refcount;
-    }
-    void
-    copy(PointerHolder const& rhs)
-    {
-        this->init(rhs.data);
-    }
-    void
-    destroy()
-    {
-        bool gone = false;
-        {
-            if (--this->data->refcount == 0) {
-                gone = true;
-            }
-        }
-        if (gone) {
-            delete this->data;
-        }
-    }
-
-    Data* data;
 };
 
 template <typename T, typename... _Args>
@@ -361,4 +246,5 @@ make_array_pointer_holder(size_t n)
     return PointerHolder<T>(true, new T[n]);
 }
 
+#endif // POINTERHOLDER_TRANSITION < 4
 #endif // POINTERHOLDER_HH
diff --git a/libtests/pointer_holder.cc b/libtests/pointer_holder.cc
index 70c83aeb..f03c4257 100644
--- a/libtests/pointer_holder.cc
+++ b/libtests/pointer_holder.cc
@@ -65,8 +65,8 @@ callHelloWithGet(ObjectHolder const& oh)
     (*oh).hello();
 }
 
-int
-main(int argc, char* argv[])
+void
+test_ph()
 {
     std::list<ObjectHolder> ol1;
 
@@ -111,5 +111,135 @@ main(int argc, char* argv[])
     std::cout << "array" << std::endl;
     PointerHolder<Object> o_arr1_ph(true, new Object[2]);
     std::cout << "goodbye" << std::endl;
+}
+
+PointerHolder<Object>
+make_object_ph()
+{
+    return new Object;
+}
+
+std::shared_ptr<Object>
+make_object_sp()
+{
+    return std::make_shared<Object>();
+}
+
+PointerHolder<Object const>
+make_object_const_ph()
+{
+    return new Object;
+}
+
+std::shared_ptr<Object const>
+make_object_const_sp()
+{
+    return std::make_shared<Object const>();
+}
+
+void
+hello_ph(PointerHolder<Object> o)
+{
+    o->hello();
+}
+
+void
+hello_sp(std::shared_ptr<Object> o)
+{
+    o->hello();
+}
+
+void
+hello_ph_const(PointerHolder<Object const> o)
+{
+    o->hello();
+}
+
+void
+hello_sp_const(std::shared_ptr<Object const> o)
+{
+    o->hello();
+}
+
+void
+ph_sp_compat()
+{
+    // Ensure bidirectional compatibility between PointerHolder and
+    // shared_ptr.
+    std::cout << "compat" << std::endl;
+    PointerHolder<Object> ph_from_ph = make_object_ph();
+    std::shared_ptr<Object> sp_from_ph = make_object_ph();
+    PointerHolder<Object> ph_from_sp = make_object_sp();
+    std::shared_ptr<Object> sp_from_sp = make_object_sp();
+    hello_sp(ph_from_ph);
+    hello_ph(sp_from_ph);
+    hello_sp(ph_from_sp);
+    hello_ph(sp_from_sp);
+    PointerHolder<Object const> ph_const_from_ph = make_object_const_ph();
+    std::shared_ptr<Object const> sp_const_from_ph = make_object_const_ph();
+    PointerHolder<Object const> ph_const_from_sp = make_object_const_sp();
+    std::shared_ptr<Object const> sp_const_from_sp = make_object_const_sp();
+    hello_sp_const(ph_const_from_ph);
+    hello_ph_const(sp_const_from_ph);
+    hello_sp_const(ph_const_from_sp);
+    hello_ph_const(sp_const_from_sp);
+    PointerHolder<Object> arr1_ph;
+    {
+        std::cout << "initialize ph array from shared_ptr" << std::endl;
+        std::shared_ptr<Object> arr1(
+            new Object[2], std::default_delete<Object[]>());
+        arr1_ph = arr1;
+    }
+    std::cout << "delete ph array" << std::endl;
+    arr1_ph = nullptr;
+    std::shared_ptr<Object> arr2_sp;
+    {
+        std::cout << "initialize sp array from PointerHolder" << std::endl;
+        PointerHolder<Object> arr2(true, new Object[2]);
+        arr2_sp = arr2;
+    }
+    std::cout << "delete sp array" << std::endl;
+    arr2_sp = nullptr;
+    std::cout << "end compat" << std::endl;
+}
+
+std::list<PointerHolder<Object>>
+get_ph_list()
+{
+    std::list<PointerHolder<Object>> l = {
+        make_object_sp(),
+        make_object_ph(),
+    };
+    return l;
+}
+
+std::list<std::shared_ptr<Object>>
+get_sp_list()
+{
+    std::list<std::shared_ptr<Object>> l = {
+        make_object_sp(),
+        make_object_ph(),
+    };
+    return l;
+}
+
+void
+ph_sp_containers()
+{
+    std::cout << "containers" << std::endl;
+    // Demonstrate that using auto makes it easy to switch interfaces
+    // from using a container of one shared pointer type to a
+    // container of the other.
+    auto phl1 = get_ph_list();
+    auto phl2 = get_sp_list();
+    std::cout << "end containers" << std::endl;
+}
+
+int
+main(int argc, char* argv[])
+{
+    test_ph();
+    ph_sp_compat();
+    ph_sp_containers();
     return 0;
 }
diff --git a/libtests/qtest/ph/ph.out b/libtests/qtest/ph/ph.out
index a7efe1bb..7b8ba935 100644
--- a/libtests/qtest/ph/ph.out
+++ b/libtests/qtest/ph/ph.out
@@ -30,3 +30,51 @@ destroyed Object, id 6
 destroyed Object, id 5
 destroyed Object, id 3
 destroyed Object, id 1
+compat
+created Object, id 7
+created Object, id 8
+created Object, id 9
+created Object, id 10
+calling Object::hello for 7
+calling Object::hello for 8
+calling Object::hello for 9
+calling Object::hello for 10
+created Object, id 11
+created Object, id 12
+created Object, id 13
+created Object, id 14
+calling Object::hello const for 11
+calling Object::hello const for 12
+calling Object::hello const for 13
+calling Object::hello const for 14
+initialize ph array from shared_ptr
+created Object, id 15
+created Object, id 16
+delete ph array
+destroyed Object, id 16
+destroyed Object, id 15
+initialize sp array from PointerHolder
+created Object, id 17
+created Object, id 18
+delete sp array
+destroyed Object, id 18
+destroyed Object, id 17
+end compat
+destroyed Object, id 14
+destroyed Object, id 13
+destroyed Object, id 12
+destroyed Object, id 11
+destroyed Object, id 10
+destroyed Object, id 9
+destroyed Object, id 8
+destroyed Object, id 7
+containers
+created Object, id 19
+created Object, id 20
+created Object, id 21
+created Object, id 22
+end containers
+destroyed Object, id 21
+destroyed Object, id 22
+destroyed Object, id 19
+destroyed Object, id 20