Make Storable::cloneStorable return shared_ptr.

While unique_ptr is a better return type for a factory method, a
unique_ptr return cannot be implemented in terms of a shared_ptr
return (though the reverse is true), and our existing classes cannot
be modified to clone by unique_ptr without running into a pybind11 bug
(pybind/pybind11#1138). At present it looks
like the pybind11 team is leaning towards forbidding such code rather
than supporting unique_ptr to shared_ptr conversion.

Author: kfindeisen

include/lsst/afw/typehandling/PolymorphicValue.h

@@ -137,7 +137,8 @@ class PolymorphicValue final {
     std::size_t hash_value() const;
 
 private:
-    std::unique_ptr<Storable> _value;
+    // unique_ptr would be more appropriate, but Storable::cloneStorable must return shared_ptr
+    std::shared_ptr<Storable> _value;
 };
 
 }  // namespace typehandling

include/lsst/afw/typehandling/Storable.h

@@ -73,7 +73,8 @@ class Storable : public table::io::Persistable {
      * @note If this class supports a `clone` operation, the two should behave
      *       identically except for the formal return type.
      */
-    virtual std::unique_ptr<Storable> cloneStorable() const;
+    // Return shared_ptr to work around https://github.com/pybind/pybind11/issues/1138
+    virtual std::shared_ptr<Storable> cloneStorable() const;
 
     /**
      * Create a string representation of this object (optional operation).

include/lsst/afw/typehandling/test.h

@@ -59,7 +59,7 @@ class SimpleStorable : public Storable {
 public:
     virtual ~SimpleStorable() = default;
 
-    std::unique_ptr<Storable> cloneStorable() const override { return std::make_unique<SimpleStorable>(); }
+    std::shared_ptr<Storable> cloneStorable() const override { return std::make_unique<SimpleStorable>(); }
 
     std::string toString() const override { return "Simplest possible representation"; }
 
@@ -80,7 +80,7 @@ class ComplexStorable final : public SimpleStorable {
         return *this;
     }
 
-    std::unique_ptr<Storable> cloneStorable() const override {
+    std::shared_ptr<Storable> cloneStorable() const override {
         return std::make_unique<ComplexStorable>(storage);
     }
 

src/typehandling/PolymorphicValue.cc

@@ -32,12 +32,12 @@ namespace typehandling {
 
 PolymorphicValue::PolymorphicValue(Storable const& value) : _value(value.cloneStorable()) {}
 // No move-constructor, because putting a pointer to Storable&& into a
-// unique_ptr is safe only if the object was dynamically allocated
+// shared_ptr is safe only if the object was dynamically allocated
 
 PolymorphicValue::~PolymorphicValue() noexcept = default;
 
 PolymorphicValue::PolymorphicValue(PolymorphicValue const& other)
-        : _value(other._value ? other._value->cloneStorable() : std::unique_ptr<Storable>()) {}
+        : _value(other._value ? other._value->cloneStorable() : nullptr) {}
 PolymorphicValue::PolymorphicValue(PolymorphicValue&&) = default;  // other._value emptied
 
 PolymorphicValue& PolymorphicValue::operator=(PolymorphicValue const& other) {

src/typehandling/Storable.cc

@@ -33,7 +33,7 @@ namespace typehandling {
 
 Storable::~Storable() noexcept {}
 
-std::unique_ptr<Storable> Storable::cloneStorable() const {
+std::shared_ptr<Storable> Storable::cloneStorable() const {
     throw LSST_EXCEPT(UnsupportedOperationException, "Cloning is not supported.");
 }