==

UPDATED: next revision HERE

==

My previous two iterations were here and here. I've since finalized the concept as described in the title, and would appreciate any feedback on the new solution located on GitHub.

Introduction:

value_ptr is a C++11 header only, deep-copying smart pointer that preserves value semantics for both polymorphic and undefined types. value_ptr aims to address the following issues by reducing/eliminating the boilerplate needed to facilitate value semantics: The polymorphic copy problem, and the undefined type problem.

The polymorphic copy problem. Given a class hierarchy, preserve value semantics while preventing object slicing.

Example:

Without value_ptr:

struct Base { virtual Base* clone() const { return new Base(*this); } };  
struct Derived : Base { Base* clone() const { return new Derived(*this); };

struct MyAwesomeClass {
    std::unique_ptr<Base> foo;
};

int main() {
    MyAwesomeClass a{};
    // lets make a copy of a
    auto b = a; // ERROR.  Now we have to do a bunch of boilerplate to clone 'foo', etc.  Boo!
}

With value_ptr:

#include "value_ptr.hpp"
struct Base { virtual Base* clone() const { return new Base(*this); } };  
struct Derived : Base { Base* clone() const { return new Derived(*this); };

struct MyAwesomeClass {
    smart_ptr::value_ptr<Base> foo;
};

int main() {
    MyAwesomeClass a{};
    // lets make a copy of a
    auto b = a; // no boilerplate, no slicing, no problem.  yay!
}

The undefined type problem. Given a declared-but-not-yet defined type (which may or may not be polymorphic), preserve value semantics and prevent object slicing. This problem is often seen in the PIMPL idiom and often associated with forward declared classes.

Example:

Without value_ptr:

struct U;   // undefined type, defined in another file somewhere that we can't/don't want to include
class MyAwesomeClass {
    std::unique_ptr<U> u; // unique_ptr doesn't really fit, but we don't want to manage a raw pointer either.
};

MyAwesomeClass a{};
auto b = a; // ERROR C2280!

With value_ptr:

struct U;   // undefined type
class MyAwesomeClass {
    smart_ptr::value_ptr<U> u;
};

MyAwesomeClass a{};
auto b = a; // no problem!

Features:

  • Header only, single file, cross platform, no dependencies outside the STL
  • Compatible interface/convertible to std::unique_ptr
  • Space efficient:
    • Utilizes empty base optimization to minimize memory footprint
    • defined types: sizeof( value_ptr ) == sizeof(T*) == sizeof(std::unique_ptr)
    • Undefined types: sizeof( value_ptr ) == sizeof(T*) + two function pointers
  • Polymorphic copying:
    • Automatically detects/utilizes clone() member function
  • Static assertion prevents object slicing if a user-defined copier not provided or clone member not found
  • Support for stateful and stateless deleters and copiers, via functors or lambdas
  • Unit tested, valgrind clean

The code:

// Distributed under the Boost Software License, Version 1.0.
//    (See http://www.boost.org/LICENSE_1_0.txt)

#ifndef _SMART_PTR_VALUE_PTR_
#define _SMART_PTR_VALUE_PTR_

#include <memory>       // unique_ptr
#include <functional>   // less_equal
#include <cassert>      // assert

#if defined( _MSC_VER)  // todo:  check constexpr/delegating ctor issue in vs17.  issue persists in vs15 update 3 despite ms closed bug as fixed, or i'm doing something wrong
#define VALUE_PTR_CONSTEXPR 
#else
#define VALUE_PTR_CONSTEXPR constexpr 
#endif

namespace smart_ptr {

    namespace detail {

        // void_t for c++11
        //  from http://en.cppreference.com/w/cpp/types/void_t
        template<typename... Ts> struct make_void { typedef void type; };
        template<typename... Ts> using void_t = typename make_void<Ts...>::type;

        // is_defined<T>, from https://stackoverflow.com/a/39816909
        template <class, class = void> struct is_defined : std::false_type { };
        template <class T> struct is_defined<
            T
            , typename std::enable_if<std::is_object<T>::value && !std::is_pointer<T>::value && ( sizeof( T ) > 0 )>::type
            >
            : std::true_type{}
        ;

        // Class function/type detection
        //  https://stackoverflow.com/a/30848101

        // Primary template handles all types not supporting the operation.
        template <typename, template <typename> class, typename = void_t<>>
        struct detect : std::false_type {};

        // Specialization recognizes/validates only types supporting the archetype.
        template <typename T, template <typename> class Op>

        struct detect<T, Op, void_t<Op<T>>> : std::true_type {};

        // clone function
        template <typename T> using fn_clone_t = decltype( std::declval<T>().clone() );

        // has_clone
        template <typename T> using has_clone = detect<T, fn_clone_t>;

        // Returns flag if test passes (false==slicing is probable)
        // T==base pointer, U==derived/supplied pointer
        template <typename T, typename U, bool IsDefaultCopier>
        struct slice_test : std::conditional<
            std::is_same<T, U>::value   // if U==T, no need to check for slicing
            || std::is_same<std::nullptr_t, U>::value   // nullptr is fine
            || !IsDefaultCopier // user provided cloner, assume they're handling it
            || has_clone<typename std::remove_pointer<U>::type>::value  // using default cloner, clone method must exist in U
            , std::true_type
            , std::false_type
        >::type {};

        // op_wrapper wraps Op::operator() and dispatches to observer fn
        //  observer fn then calls op_wrapper.op() to invoke Op::operator()
        //  this redirection is needed to call the actual operation in a context when T is actually defined
        template <typename T, typename Op, typename R, typename ObserverFnSig>
        struct op_wrapper : public Op {
            using this_type = op_wrapper<T, Op, R, ObserverFnSig>;
            using return_type = R;

            // observer function to call
            ObserverFnSig observer_fn;

            template <typename Op_, typename Fn>
            constexpr op_wrapper( Op_&& op, Fn&& obs )
                : Op( std::forward<Op_>( op ) )
                , observer_fn( std::forward<Fn>( obs ) )
            {}

            // invoked for event
            template <typename... Args>
            return_type operator()( Args&&... args ) const {
                assert( this->observer_fn != nullptr );
                //  here we want to notify observer of event, with reference to this as first parameter
                return this->observer_fn( (const void*)this, std::forward<Args>( args )... );
            }

            // call to actual operation (Op::operator()), invoked by observer
            template <typename... Args>
            return_type op( Args&&... args ) const {
                return Op::operator()( std::forward<Args>(args)... );
            }

        };  // op_wrapper

        // ptr_data
        template <typename T, typename Deleter, typename Copier>
            struct
#ifdef _MSC_VER
            //  https://blogs.msdn.microsoft.com/vcblog/2016/03/30/optimizing-the-layout-of-empty-base-classes-in-vs2015-update-2-3/
            __declspec( empty_bases )   // requires vs2015 update 2
#endif
            ptr_data
            : public std::unique_ptr<T, Deleter>
            , public Copier
        {
            using copier_type = Copier;
            using base_type_uptr = std::unique_ptr<T, Deleter>;
            using deleter_type = Deleter;

            ptr_data() = default;

            template <typename Dx, typename Cx>
            constexpr ptr_data( T* px, Dx&& dx, Cx&& cx ) noexcept
                : base_type_uptr( px, std::forward<Dx>(dx) )
                , copier_type( std::forward<Cx>(cx) )
            {}

            copier_type& get_copier() { return static_cast<copier_type&>( *this ); }
            const copier_type& get_copier() const { return static_cast<const copier_type&>( *this ); }

            ptr_data clone() const {
                return{ this->get_copier()( this->get() ), this->get_deleter(), this->get_copier() };
            }

            ptr_data( ptr_data&& ) = default;
            ptr_data& operator=( ptr_data&& ) = default;
            ptr_data( const ptr_data& that )
                : ptr_data( that.clone() )
            {}

            ptr_data& operator=( const ptr_data& that ) {
                if ( this == &that )
                    return *this;
                *this = that.clone();
                return *this;
            }

        };  // ptr_data

        // ptr_base:    base class for defined types
        template <typename T, typename Deleter, typename Copier>
        struct ptr_base {

            using _data_type = ptr_data<T, Deleter, Copier>;
            using _pointer = typename _data_type::pointer;
            _data_type _data;

            using pointer = _pointer;

            template <typename Px, typename Dx, typename Cx>
            constexpr ptr_base( Px&& px, Dx&& dx, Cx&& cx )
                : _data(
                    std::forward<Px>( px )
                    , std::forward<Dx>( dx )
                    , std::forward<Cx>(cx)
                )
            {}

            // conversion to unique_ptr
            const typename _data_type::base_type_uptr& ptr() const {
                return this->_data;
            }

            // conversion to unique_ptr
            typename _data_type::base_type_uptr& ptr() {
                return this->_data;
            }

            // conversion to unique_ptr
            operator typename _data_type::base_type_uptr const&() const {
                return this->_data;
            }

            // conversion to unique_ptr
            operator typename _data_type::base_type_uptr& () {
                return this->_data;
            }


        };  // ptr_base

        // ptr_base_undefined:  intermediate base class for undefined types
        template <typename T, typename DeleteOp, typename CopyOp
            , typename Deleter = op_wrapper<T, DeleteOp, void, void( *)( const void*, T* )>
            , typename Copier = op_wrapper<T, CopyOp, T*, T*(*)(const void*, const T*)>
        >
        struct ptr_base_undefined 
            : ptr_base<T, Deleter, Copier> {

            using base_type = ptr_base<T,Deleter,Copier>;
            using pointer = typename base_type::pointer;

            // default construct for undefined type
            template <typename Dx, typename Cx>
            constexpr ptr_base_undefined( std::nullptr_t, Dx&& dx, Cx&& cx )
                : base_type(
                    nullptr
                    , Deleter( std::forward<Dx>( dx ), []( const void*, T* ptr ) { assert( ptr == nullptr ); } )
                    , Copier( std::forward<Cx>( cx ), []( const void* op, const T* ptr ) -> T* { assert( ptr == nullptr ); return nullptr; } )
                )
            {}

            template <typename Dx, typename Cx>
            constexpr ptr_base_undefined( pointer px, Dx&& dx, Cx&& cx )
                : base_type(
                    px
                    , Deleter( std::forward<Dx>( dx ), []( const void* op, T* ptr ) {
                            if ( ptr )
                                static_cast<const Deleter*>( op )->op( ptr );
                        }
                    )
                    , Copier( std::forward<Cx>( cx ), []( const void* op, const T* ptr ) -> T* {
                            if ( !ptr )
                                return nullptr;
                            return static_cast<const Copier*>( op )->op( ptr );
                        }
                    )
                )
            {}
        };  // ptr_base_undefined

    }   // detail

    template <typename T>
    struct default_copy {

        // copy operator
        T *operator()( const T* what ) const {

            if ( !what )
                return nullptr;

            // tag dispatch on has_clone
            return this->operator()( what
                , typename std::conditional<detail::has_clone<T>::value, _clone, _copy>::type() 
                );
        }   // operator()

    private:
        struct _clone {};
        struct _copy {};

        T* operator()( const T* what, _clone ) const {
            return what->clone();
        }

        T* operator()( const T* what, _copy ) const {
            return new T( *what );
        }   // _copy

    };  // default_copy

    template <typename T
        , typename Deleter = std::default_delete<T>
        , typename Copier = default_copy<T>
        , typename Base = 
            typename std::conditional<detail::is_defined<T>::value, 
                detail::ptr_base<T, Deleter, Copier>
                , detail::ptr_base_undefined<T, Deleter, Copier>
            >::type
    >
        struct value_ptr 
            : Base
        {
            using base_type = Base;
            using element_type = T;

            using pointer = typename base_type::pointer;
            using const_pointer = const pointer;

            using reference = typename std::add_lvalue_reference<element_type>::type;
            using const_reference = const reference;

            using deleter_type = Deleter;
            using copier_type = Copier;

            // construct with pointer, deleter, copier
            template <typename Px>
            constexpr value_ptr( Px px, deleter_type dx, copier_type cx )
                : base_type( px
                    , std::move( dx )
                    , std::move( cx )
                )
            {
                static_assert(
                    detail::slice_test<pointer, Px, std::is_same<default_copy<T>, copier_type>::value>::value
                    , "value_ptr; clone() method not detected and not using custom copier; slicing may occur"
                    );
            }

            // construct with pointer, deleter
            template <typename Px>
            VALUE_PTR_CONSTEXPR value_ptr( Px px, deleter_type dx ) // constexpr here yields c2476 on msvc15
                : value_ptr( px, std::move(dx), copier_type() )
            {}

            // construct with pointer
            template <typename Px>
            VALUE_PTR_CONSTEXPR value_ptr( Px px ) // constexpr here yields c2476 on msvc15
                : value_ptr( px, deleter_type(), copier_type() )
            {}

            // std::nullptr_t, default ctor 
            explicit VALUE_PTR_CONSTEXPR value_ptr( std::nullptr_t = nullptr )  // constexpr here yields c2476 on msvc15
                : value_ptr( nullptr, deleter_type(), copier_type() )
            {}

            // get pointer
            pointer get() { return this->_data.get(); }

            // get const pointer
            const_pointer get() const { return this->_data.get(); }

            // reset pointer
            template <typename Px = std::nullptr_t>
            void reset( Px px = nullptr ) {

                static_assert(
                    detail::slice_test<pointer, Px, std::is_same<default_copy<T>, copier_type>::value>::value
                    , "value_ptr; clone() method not detected and not using custom copier; slicing may occur"
                    );

                *this = value_ptr( px, this->get_deleter(), this->get_copier() );
            }

            // release pointer
            pointer release() noexcept {
                return this->_data.release();
            }   // release

            // return flag if has pointer
            explicit operator bool() const {
                return this->get() != nullptr;
            }

            const_reference operator*() const { return *this->get(); }
            reference operator*() { return *this->get(); }

            const_pointer operator-> () const { return this->get(); }
            pointer operator-> () { return this->get(); }

            void swap( value_ptr& that ) { std::swap( this->_data, that._data ); }

            deleter_type& get_deleter() { return this->_data.get_deleter(); }
            const deleter_type& get_deleter() const { return this->_data.get_deleter(); }

            copier_type& get_copier() { return this->_data.get_copier(); }
            const copier_type& get_copier() const { return this->_data.get_copier(); }

    };// value_ptr

      // non-member swap
    template <class T1, class D1, class C1, class T2, class D2, class C2> void swap( value_ptr<T1, D1, C1>& x, value_ptr<T2, D2, C2>& y ) { x.swap( y ); }

    // non-member operators
    template <class T1, class D1, class C1, class T2, class D2, class C2> bool operator == ( const value_ptr<T1, D1, C1>& x, const value_ptr<T2, D2, C2>& y ) { return x.get() == y.get(); }
    template<class T1, class D1, class C1, class T2, class D2, class C2> bool operator != ( const value_ptr<T1, D1, C1>& x, const value_ptr<T2, D2, C2>& y ) { return x.get() != y.get(); }
    template<class T1, class D1, class C1, class T2, class D2, class C2> bool operator < ( const value_ptr<T1, D1, C1>& x, const value_ptr<T2, D2, C2>& y ) {
        using common_type = typename std::common_type<typename value_ptr<T1, D1, C1>::pointer, typename value_ptr<T2, D2, C2>::pointer>::type;
        return std::less<common_type>()( x.get(), y.get() );
    }
    template<class T1, class D1, class C1, class T2, class D2, class C2> bool operator <= ( const value_ptr<T1, D1, C1>& x, const value_ptr<T2, D2, C2>& y ) { return !( y < x ); }
    template<class T1, class D1, class C1, class T2, class D2, class C2> bool operator >( const value_ptr<T1, D1, C1>& x, const value_ptr<T2, D2, C2>& y ) { return y < x; }
    template<class T1, class D1, class C1, class T2, class D2, class C2> bool operator >= ( const value_ptr<T1, D1, C1>& x, const value_ptr<T2, D2, C2>& y ) { return !( x < y ); }
    template <class T, class D, class C> bool operator == ( const value_ptr<T, D, C>& x, std::nullptr_t ) noexcept { return !x; }
    template <class T, class D, class C> bool operator == ( std::nullptr_t, const value_ptr<T, D, C>& x ) noexcept { return !x; }
    template <class T, class D, class C> bool operator != ( const value_ptr<T, D, C>& x, std::nullptr_t ) noexcept { return (bool)x; }
    template <class T, class D, class C> bool operator != ( std::nullptr_t, const value_ptr<T, D, C>& x ) noexcept { return (bool)x; }
    template <class T, class D, class C> bool operator < ( const value_ptr<T, D, C>& x, std::nullptr_t ) { return std::less<typename value_ptr<T, D, C>::pointer>()( x.get(), nullptr ); }
    template <class T, class D, class C> bool operator<( std::nullptr_t, const value_ptr<T, D, C>& y ) { return std::less<typename value_ptr<T, D, C>::pointer>()( nullptr, y.get() ); }
    template <class T, class D, class C> bool operator <= ( const value_ptr<T, D, C>& x, std::nullptr_t ) { return std::less_equal<typename value_ptr<T, D, C>::pointer>()( x.get(), nullptr ); }
    template <class T, class D, class C> bool operator <= ( std::nullptr_t, const value_ptr<T, D, C>& y ) { return std::less_equal<typename value_ptr<T, D, C>::pointer>()( nullptr, y.get() ); }
    template <class T, class D, class C> bool operator >( const value_ptr<T, D, C>& x, std::nullptr_t ) { return !( nullptr < x ); }
    template <class T, class D, class C> bool operator > ( std::nullptr_t, const value_ptr<T, D, C>& y ) { return !( y < nullptr ); }
    template <class T, class D, class C> bool operator >= ( const value_ptr<T, D, C>& x, std::nullptr_t ) { return !( x < nullptr ); }
    template <class T, class D, class C> bool operator >= ( std::nullptr_t, const value_ptr<T, D, C>& y ) { return !( nullptr < y ); }

    template <typename T, typename Deleter>
    static inline auto make_value_ptr( T* ptr, Deleter&& dx ) -> value_ptr<T, Deleter> {
        return value_ptr<T, Deleter>( ptr, std::forward<Deleter>( dx ) );
    }   // make_value_ptr

    template <typename T, typename Deleter, typename Copier>
    static inline auto make_value_ptr( T* ptr, Deleter&& dx, Copier&& cx ) -> value_ptr<T, Deleter, Copier> {
        return value_ptr<T, Deleter, Copier>( ptr, std::forward<Deleter>( dx ), std::forward<Copier>( cx ) );
    }   // make_value_ptr

}   // smart_ptr ns

#undef VALUE_PTR_CONSTEXPR

#endif // !_SMART_PTR_VALUE_PTR_
  • 1
    Shame nobody seems to have time to review this, it’s a very nice submission. Unfortunately I lack the time at the moment; just one thing I noticed; the identifier _SMART_PTR_VALUE_PTR_ is reserved for the implementation, and is illegal in client code; the rule is: everything in the global namespace starting with __ (double underscore) or _ (single underscore) + uppercase letter is reserved. – Konrad Rudolph Jul 14 '17 at 14:33
  • @KonradRudolph - thank you! I didn't know that about the leading underscore – Tom Jul 14 '17 at 16:33
  • Wow. I had never heard of __declspec( empty_bases ) before now. That's something I'm gonna have to use. – Justin Aug 2 '17 at 21:10
  • 1
    @Justin, there is standard solution coming, IIRC it is called [[no_unique_address]]. – Incomputable Oct 30 at 5:33
  • 2
    I rolled back your last edit. After getting an answer you are not allowed to change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information – Sᴀᴍ Onᴇᴌᴀ Dec 6 at 18:38
up vote 4 down vote accepted

Scanning your description: The idea of this value_ptr sounds perfectly reasonable. You have a pointer to a polymorphic object, and then you want to be able to make a copy of that object, without necessarily knowing what its "most derived type" is. You can't do that without help; so we ask the derived type to help us out: we give the base type a virtual method clone() through which we can ask the derived type to copy itself.

Your example shows clone() returning an owning raw pointer:

struct Base { virtual Base* clone() const { return new Base(*this); } };  
struct Derived : Base { Base* clone() const { return new Derived(*this); };

I strongly recommend making the signature std::unique_ptr<Base> clone() const, so that the owning nature of the return value (and the fact that it is the sole owner) is expressed directly in the code instead of implicitly. The one possible downside of using unique_ptr is that you can no longer use covariant return types:

struct Base { virtual Base* clone() const; };
struct Derived : Base { Derived* clone() const override; };  // OK

struct Base { virtual std::unique_ptr<Base> clone() const; };
struct Derived : Base { std::unique_ptr<Derived> clone() const override; };  // error!

But this isn't a big downside in your case.


The other stylistic thing to notice about my example code above is that I'm using override to tell the compiler that I intend to override a virtual method from one of my base classes, and so it should complain if my intention isn't being carried out for some reason.

struct Base { virtual Base* clone() const; };
struct Derived : Base { Derived* clone(); };  // Does The Wrong Thing at runtime

struct Base { virtual Base* clone() const; };
struct Derived : Base { Derived* clone() override; };  // error! hooray!

Finally — although I'm sure this was just an oversight in toy example code — don't forget that Base's destructor should be virtual, given that you're eventually going to be deleting a Derived object via a pointer of type Base*.


    // is_defined<T>, from https://stackoverflow.com/a/39816909
    template <class, class = void> struct is_defined : std::false_type { };
    template <class T> struct is_defined<
        T
        , typename std::enable_if<std::is_object<T>::value && !std::is_pointer<T>::value && ( sizeof( T ) > 0 )>::type
        >
        : std::true_type{}
    ;

Ooh. You really don't want to be doing this. First of all, I think the "proper" name for this type-trait would be (the inverse of) is_incomplete<T>; types don't really get "defined" per se. Or, okay, class types do; but for example void and int[] are types that are "defined" but still "incomplete," and they're the kinds of types you're catching with this type-trait.

Secondly, why don't you want to be doing this? Well, because the intended value of this trait can change over the course of a translation unit; but the actual value of the static data member cannot change.

struct A;
static_assert(is_incomplete<A>::value, "If this assertion succeeds...");
struct A {};
static_assert(not is_incomplete<A>::value, "...then this one MUST fail!");

(Godbolt.)

So don't do this. Trust your library-user to use your type correctly, and trust the compiler to give them a reasonable diagnostic if they misuse it. Don't try to dispatch on evanescent properties such as "completeness."

Finally, a nit on whitespace:

        : std::true_type{}
    ;

The construct std::true_type{} has a well-known meaning to C++11 metaprogrammers: it means "give me an object of type std::true_type." What you mean in this case is not that — you mean "...inherits from true_type, and here's the class body, which happens to be empty." So use your whitespace to indicate that.

    : std::true_type {};

// or even

    : std::true_type
{};

template <typename, template <typename> class, typename = void_t<>>
struct detect : std::false_type {};

That first line is a complicated way of writing

template<class, template<class> class, class = void>

Let's look at this whole snippet:

    // Class function/type detection
    //  https://stackoverflow.com/a/30848101

    // Primary template handles all types not supporting the operation.
    template <typename, template <typename> class, typename = void_t<>>
    struct detect : std::false_type {};

    // Specialization recognizes/validates only types supporting the archetype.
    template <typename T, template <typename> class Op>

    struct detect<T, Op, void_t<Op<T>>> : std::true_type {};

    // clone function
    template <typename T> using fn_clone_t = decltype( std::declval<T>().clone() );

    // has_clone
    template <typename T> using has_clone = detect<T, fn_clone_t>;

This is an insanely complicated way of writing what should be a two-liner:

    template<class T, class = void> struct has_clone : std::false_type {};
    template<class T> struct has_clone<T, decltype(void(std::declval<T>().clone()))> : std::true_type {};

Consider this line of metaprogramming. (I'm not going to worry about what it does, for now.)

    template <typename T, typename U, bool IsDefaultCopier>
    struct slice_test : std::conditional<
        std::is_same<T, U>::value   // if U==T, no need to check for slicing
        || std::is_same<std::nullptr_t, U>::value   // nullptr is fine
        || !IsDefaultCopier // user provided cloner, assume they're handling it
        || has_clone<typename std::remove_pointer<U>::type>::value  // using default cloner, clone method must exist in U
        , std::true_type
        , std::false_type
    >::type {};

Would you ever write

return ((t == u) || (nullptr == u) || !isdefaultcopier || has_clone(u))
    ? true : false;

? No? Then you shouldn't write the metaprogramming equivalent of return x ? true : false; either. Just return the original boolean condition itself:

    template <typename T, typename U, bool IsDefaultCopier>
    struct slice_test : std::bool_constant<
        std::is_same_v<T, U>   // if U==T, no need to check for slicing
        || std::is_same_v<std::nullptr_t, U>   // nullptr is fine
        || !IsDefaultCopier // user provided cloner, assume they're handling it
        || has_clone<std::remove_pointer_t<U>>::value  // using default cloner, clone method must exist in U
    > {};

(For C++11, use std::integral_constant<bool, XYZ> in place of std::bool_constant<XYZ>, and re-expand all my _ts and _vs. I shrank them here just to demonstrate how code should look if portability-to-C++11 is not a concern.)


        struct
#ifdef _MSC_VER
        //  https://blogs.msdn.microsoft.com/vcblog/2016/03/30/optimizing-the-layout-of-empty-base-classes-in-vs2015-update-2-3/
        __declspec( empty_bases )   // requires vs2015 update 2
#endif
        ptr_data

I strongly recommend that you move the preprocessor logic to the top of the file, use it to set a macro, and use just the macro down here. It'll be easier to read and easier to maintain.

#ifdef _MSC_VER
#define USE_EMPTY_BASE_OPTIMIZATION __declspec(empty_bases)
#else
#define USE_EMPTY_BASE_OPTIMIZATION
#endif

// ...

struct USE_EMPTY_BASE_OPTIMIZATION ptr_data

        : public std::unique_ptr<T, Deleter>

Inheriting (either publicly or privately) from standard-library types is never a good idea. I strongly recommend that you just implement the three special members (move-constructor, move-assignment, and destructor) yourself, rather than trying to reuse the STL's versions. ...Oh look, you do reimplement them, anyway!

        ptr_data& operator=( const ptr_data& that ) {
            if ( this == &that )
                return *this;
            *this = that.clone();  // R
            return *this;
        }

It's been a while since you wrote this code. How long will it take you to prove to yourself that the line marked // R is not a recursive call to this operator=? (Or is it? ;) Remember all of the behaviors of unique_ptr!)


Speaking of which, inheriting from unique_ptr<T, Deleter> is also kind of silly since unique_ptr<T, Deleter> will have a data member of type Deleter::pointer (which may be a fancy pointer type), whereas your clone() methods seem to deal only in raw T*. If you don't want the fancy-pointer behavior, you shouldn't drag it in. Yet another reason to implement the special members yourself instead of inheriting from unique_ptr.


        // tag dispatch on has_clone
        return this->operator()( what
            , typename std::conditional<detail::has_clone<T>::value, _clone, _copy>::type() 
            );

You seem to be a bit operator()-happy in this code. There's no reason for this overload set to be named operator(); I'd recommend something descriptive such as copy_or_clone.

General rule of thumb: If you find yourself writing this->operator()(...) instead of (*this)(...), then you are definitely doing it wrong. (And if you find yourself writing (*this)(...), you are probably doing it wrong, anyway.)

I also strongly recommend to avoid leading underscores. In this case, since they are tag types, _copy should be spelled copy_tag and _clone should be spelled clone_tag.


Did you consider using std::function<Base*(Base*)> m_clone and std::function<void(Base*)> m_destroy instead of rolling your own type-erasure? I mean, I love rolling my own type-erasure, and do it all the time; but if you're just practicing metaprogramming and want the shortest possible code, you might find that std::function would be useful.

You could even use std::function<Base*(Base*, enum CloneOrDestroy)> to wrap up both behaviors into a single function, to shrink the size of your value_ptr object.

Did you consider what should happen if Base's destructor is not virtual? There is actually a type-trait for has_virtual_destructor<T>, not that I would recommend using it (see my above advice about trusting your user). But consider how shared_ptr works even with non-virtual destructors; should you do similarly?

This has been a long review already, and I didn't even really get to the meat of it. I would recommend simplifying the code a lot and re-posting for another round.

  • Thanks for the feedback! It's great. Couple points/questions: 1- I'm inheriting from unique_ptr so that value_ptr can be a drop-in replacement (and for conversion). 2- 'never inherit from std'...normally I agree but to achieve point 1 it was necessary, and "never" seems a bit strong without addt'l reasoning since this topic is debated 3- I like using operator() as 'do the thing for which you exist' for small functors and is idiomatic in c++, why doesn't it work here (see default_delete)? 4- Could you please expand on the recursive/clone part? 5- love the single clone/delete fn. Thanks again! – Tom Nov 28 at 18:20
  • 4) The line marked "R" assigns a unique_ptr to a value_ptr, right? So it would use some overloaded operator=. Considering all the possible implicit conversions, and all the candidate overloads of operator= (some of which are inherited), ask yourself: which operator= does it choose, and why? Speaking of conversion, 1) is your value_ptr convertible-to unique_ptr? is your value_ptr convertible-from unique_ptr? Which of these conversions are desirable, and why? – Quuxplusone Nov 29 at 16:54
  • On operator(): In the number of characters it took you to type this->operator()(...), I could type this->copy_clone(...). That would be more descriptive, right? Define operator() (or any overloaded operator) only if you're actually going to use the overloaded operator. If you'd written (*this)(...), I still would have complained about its unreadability... but apparently you already intuitively knew about the unreadability and didn't even consider writing (*this)(...), so we can skip straight to the difference in self-documenting-ness between operator() and copy_or_clone. – Quuxplusone Nov 29 at 16:59
  • Hmm regarding #4, the line marked "R" assigns a ptr_data to a ptr_data. ptr_data is the internal holder structure I use which derives from unique_ptr. value_ptr (or its bases) do not derive from unique_ptr. I expect that the ptr_data& operator=( ptr_data&& ) = default; line would be called as a result of this operation, since clone returns a prvalue and not a const&. I'm not seeing how this could ever be recursive unless I'm missing something? – Tom Nov 29 at 17:43
  • 1
    Updated! ..... 15chars – Tom Dec 6 at 18:16

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.