Rework PointerHolder transition to make it smoother

* Don't surprise people with deprecation warnings
* Provide detailed instructions and support for the transition

1 files changed, 229 insertions, 89 deletions

diff --git a/include/qpdf/PointerHolder.hh b/include/qpdf/PointerHolder.hh
index 8e758ee6..2297ba21 100644
--- a/include/qpdf/PointerHolder.hh
+++ b/include/qpdf/PointerHolder.hh
@@ -22,13 +22,125 @@
 #ifndef POINTERHOLDER_HH
 #define POINTERHOLDER_HH
 
-// In qpdf 11, PointerHolder will be derived from std::shared_ptr and
-// will also include a fix to incorrect semantics of const
-// PointerHolder objects. PointerHolder only allows a const
-// PointerHolder to return a const pointer. This is wrong. Use a
-// PointerHolder<const T> for that. A const PointerHolder should just
-// not allow you to change what it points to. This is consistent with
-// how regular pointers and standard library shared pointers work.
+#ifndef POINTERHOLDER_TRANSITION
+
+// In qpdf 11, #define POINTERHOLDER_IS_SHARED_POINTER
+
+// In qpdf 11, issue a warning:
+// #define POINTERHOLDER_TRANSITION 0 to suppress this warning, and see below.
+// # warn "POINTERHOLDER_TRANSITION is not defined -- see qpdf/PointerHolder.hh"
+
+// undefined = define as 0; will also issue a warning in qpdf 11
+// 0 = no deprecation warnings
+// 1 = make PointerHolder<T>(T*) explicit
+// 2 = warn for use of getPointer() and getRefcount()
+// 3 = warn for all use of PointerHolder
+// 4 = don't define PointerHolder at all
+# define POINTERHOLDER_TRANSITION 0
+#endif // !defined(POINTERHOLDER_TRANSITION)
+
+// *** WHAT IS HAPPENING ***
+
+// In qpdf 11, PointerHolder will be replaced with std::shared_ptr
+// wherever it appears in the qpdf API. The PointerHolder object will
+// be derived from std::shared_ptr to provide a backward-compatible
+// interface and will be mutually assignable with std::shared_ptr.
+// Code that uses containers of PointerHolder will require adjustment.
+
+// *** HOW TO TRANSITION ***
+
+// The symbol POINTERHOLDER_TRANSITION can be defined to help you
+// transition your code away from PointerHolder. You can define it
+// before including any qpdf header files or including its definition
+// in your build configuration. If not defined, it automatically gets
+// defined to 0, which enables full backward compatibility. That way,
+// you don't have to take action for your code to continue to work.
+
+// If you want to work gradually to transition your code away from
+// PointerHolder, you can define POINTERHOLDER_TRANSITION and fix the
+// code so it compiles without warnings and works correctly. If you
+// want to be able to continue to support old qpdf versions at the
+// same time, you can write code like this:
+
+// #ifndef POINTERHOLDER_TRANSITION
+//  ... use PointerHolder as before 10.6
+// #else
+//  ... use PointerHolder or shared_ptr as needed
+// #endif
+
+// Each level of POINTERHOLDER_TRANSITION exposes differences between
+// PointerHolder and std::shared_ptr. The easiest way to transition is
+// to increase POINTERHOLDER_TRANSITION in steps of 1 so that you can
+// test and handle changes incrementally.
+
+// *** Transitions available starting at qpdf 10.6.0 ***
+
+// POINTERHOLDER_TRANSITION = 1
+//
+// PointerHolder<T> has an implicit constructor that takes a T*, so
+// you can replace a PointerHolder<T>'s pointer by directly assigning
+// a T* to it or pass a T* to a function that expects a
+// PointerHolder<T>. std::shared_ptr does not have this (risky)
+// behavior. When POINTERHOLDER_TRANSITION = 1, PointerHolder<T>'s T*
+// constructor is declared explicit. For compatibility with
+// std::shared_ptr, you can still assign nullptr to a PointerHolder.
+// Constructing all your PointerHolder<T> instances explicitly is
+// backward compatible, so you can make this change without
+// conditional compilation and still use the changes with older qpdf
+// versions.
+//
+// Also defined is a make_pointer_holder method that acts like
+// std::make_shared. You can use this as well, but it is not
+// compatible with qpdf prior to 10.6. Like std::make_shared<T>,
+// make_pointer_holder<T> can only be used when the constructor
+// implied by its arguments is public. If you use this, you should be
+// able to just replace it with std::make_shared when qpdf 11 is out.
+
+// POINTERHOLDER_TRANSITION = 2
+//
+// std::shared_ptr as get() and use_count(). PointerHolder has
+// getPointer() and getRefcount(). In 10.6.0, get() and use_count()
+// were added as well. When POINTERHOLDER_TRANSITION = 2, getPointer()
+// and getRefcount() are deprecated. Fix deprecation warnings by
+// replacing with get() and use_count(). This breaks compatibility
+// with qpdf older than 10.6. Search for CONST BEHAVIOR for an
+// additional note.
+//
+// Once your code is clean at POINTERHOLDER_TRANSITION = 2, the only
+// remaining issues that prevent simple replacement of PointerHolder
+// with std::shared_ptr are shared arrays and containers, and neither
+// of these are used in the qpdf API.
+
+// *** Transitions available starting at qpdf 11.0.0 **
+
+// NOTE: Until qpdf 11 is released, this is a plan and is subject to
+// change. Be sure to check again after qpdf 11 is released.
+
+// POINTERHOLDER_TRANSITION = 3
+//
+// Warn for all use of PointerHolder<T>. This help you remove all use
+// of PointerHolder from your code and use std::shared_ptr instead.
+// You will also have to transition any containers of PointerHolder in
+// your code.
+
+// POINTERHOLDER_TRANSITION = 4
+//
+// Suppress definition of the PointerHolder<T> type entirely.
+
+// CONST BEHAVIOR
+
+// PointerHolder<T> has had a long-standing bug in its const behavior.
+// const PointerHolder<T>'s getPointer() method returns a T const*.
+// This is incorrect and is not how regular pointers or standard
+// library smart pointers behave. Making a PointerHolder<T> const
+// should prevent reassignment of its pointer but not affect the thing
+// it points to. For that, use PointerHolder<T const>. The new get()
+// method behaves correctly in this respect and is therefore slightly
+// different from getPointer(). This shouldn't break any correctly
+// 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.
@@ -58,6 +170,8 @@
 // the underlying pointers provides a well-defined, if not
 // particularly meaningful, ordering.
 
+#include <cstddef>
+
 template <class T>
 class PointerHolder
 {
@@ -65,66 +179,78 @@ class PointerHolder
     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;
+        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;
+        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));
-	}
+    {
+        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));
-	}
+    {
+        this->init(new Data(pointer, true));
+    }
     PointerHolder(PointerHolder const& rhs)
-	{
-	    this->copy(rhs);
-	}
+    {
+        this->copy(rhs);
+    }
     PointerHolder& operator=(PointerHolder const& rhs)
-	{
-	    if (this != &rhs)
-	    {
-		this->destroy();
-		this->copy(rhs);
-	    }
-	    return *this;
-	}
+    {
+        if (this != &rhs)
+        {
+            this->destroy();
+            this->copy(rhs);
+        }
+        return *this;
+    }
+    PointerHolder& operator=(decltype(nullptr))
+    {
+        this->operator=(PointerHolder<T>());
+        return *this;
+    }
     ~PointerHolder()
-	{
-	    this->destroy();
-	}
+    {
+        this->destroy();
+    }
     bool operator==(PointerHolder const& rhs) const
     {
-	return this->data->pointer == rhs.data->pointer;
+        return this->data->pointer == rhs.data->pointer;
+    }
+    bool operator==(decltype(nullptr)) const
+    {
+        return this->data->pointer == nullptr;
     }
     bool operator<(PointerHolder const& rhs) const
     {
-	return this->data->pointer < rhs.data->pointer;
+        return this->data->pointer < rhs.data->pointer;
     }
 
     // get() is for interface compatibility with std::shared_ptr
@@ -135,33 +261,33 @@ class PointerHolder
 
     // NOTE: The pointer returned by getPointer turns into a pumpkin
     // when the last PointerHolder that contains it disappears.
-#ifndef NO_POINTERHOLDER_DEPRECATION
+#if POINTERHOLDER_TRANSITION >= 2
     [[deprecated("use PointerHolder<T>::get() instead of getPointer()")]]
-#endif
+#endif // POINTERHOLDER_TRANSITION >= 2
     T* getPointer()
-	{
-	    return this->data->pointer;
-	}
-#ifndef NO_POINTERHOLDER_DEPRECATION
+    {
+        return this->data->pointer;
+    }
+#if POINTERHOLDER_TRANSITION >= 2
     [[deprecated("use PointerHolder<T>::get() instead of getPointer()")]]
-#endif
+#endif // POINTERHOLDER_TRANSITION >= 2
     T const* getPointer() const
-	{
-	    return this->data->pointer;
-	}
-#ifndef NO_POINTERHOLDER_DEPRECATION
+    {
+        return this->data->pointer;
+    }
+#if POINTERHOLDER_TRANSITION >= 2
     [[deprecated("use use_count() instead of getRefcount()")]]
-#endif
+#endif // POINTERHOLDER_TRANSITION >= 2
     int getRefcount() const
-	{
-	    return this->data->refcount;
-	}
+    {
+        return this->data->refcount;
+    }
 
     // use_count() is for compatibility with std::shared_ptr
     long use_count()
-        {
-            return static_cast<long>(this->data->refcount);
-        }
+    {
+        return static_cast<long>(this->data->refcount);
+    }
 
     T const& operator*() const
     {
@@ -183,30 +309,44 @@ class PointerHolder
 
   private:
     void init(Data* data)
-	{
-	    this->data = data;
-            ++this->data->refcount;
-	}
+    {
+        this->data = data;
+        ++this->data->refcount;
+    }
     void copy(PointerHolder const& rhs)
-	{
-	    this->init(rhs.data);
-	}
+    {
+        this->init(rhs.data);
+    }
     void destroy()
-	{
-	    bool gone = false;
-	    {
-		if (--this->data->refcount == 0)
-		{
-		    gone = true;
-		}
-	    }
-	    if (gone)
-	    {
-		delete this->data;
-	    }
-	}
+    {
+        bool gone = false;
+        {
+            if (--this->data->refcount == 0)
+            {
+                gone = true;
+            }
+        }
+        if (gone)
+        {
+            delete this->data;
+        }
+    }
 
     Data* data;
 };
 
+template<typename T, typename... _Args>
+inline PointerHolder<T>
+make_pointer_holder(_Args&&... __args)
+{
+    return PointerHolder<T>(new T(__args...));
+}
+
+template <typename T>
+PointerHolder<T>
+make_array_pointer_holder(size_t n)
+{
+    return PointerHolder<T>(true, new T[n]);
+}
+
 #endif // POINTERHOLDER_HH