When using the pimple idiom it is necessary to represent your "impl" as a forward declared pointer in the header file. This necessitates a lot of boilerplate code to implement the rule of five.

Instead, I want to wrap std::unique_ptr with a template class DeepPtr which automatically deep copies on copy construction, assignment, and "deep" swaps on move construction and move assignment. The resulting class could also be convenient for "value-like" objects that need to be passed by handle due to polymorphism.

Areas of concern are:

  • Correct use of the swap idiom (should the local swap function be private/public or done some other way)?
  • Const correctness of the dereferencing operator overloads
  • Signature of the assignment operator function (const DeepPtr& other) which uses this const ref only to copy to temp. This seems strange, but correct.
  • Any other pitfalls that could be caused by this class.

#pragma once
#include <iostream>
#include <memory>

template <typename T>
class DeepPtr
{
public:
    DeepPtr() :myPtr( nullptr ) {}

    DeepPtr( const T& value ) :myPtr( new T{ value } ) {}

    DeepPtr( const DeepPtr& other )
    :myPtr( nullptr )
    {
        if ( other )
        {
            myPtr = std::unique_ptr<T>{ new T{ *other } };
        }
    }

    DeepPtr( DeepPtr&& other )
    :myPtr( nullptr )
    {
        if ( other )
        {
            myPtr = std::unique_ptr<T>{ new T{ *other } };
        }
    }

    DeepPtr& operator=( const DeepPtr& other )
    {
        DeepPtr temp{ other };
        swap( *this, temp );
        return *this;
    }

    DeepPtr& operator=( DeepPtr&& other )
    {
        swap( *this, other );
        return *this;
    }

    static void swap( DeepPtr& left, DeepPtr& right ) { std::swap( left.myPtr, right.myPtr ); }

    T& operator*() { return *myPtr; }

    const T& operator*() const { return *myPtr; }

    T* const operator->() { return myPtr.operator->(); }

    const T* const operator->() const { return myPtr.operator->(); }

    const T* const get() const { return myPtr.get(); }

    operator bool() const { return (bool)myPtr; }

private:
    std::unique_ptr<T> myPtr;
};

  • It's interesting that you want to be able to copy a unique_ptr which is designed to be unique, i.e., non-copyable. For my own curiosity, if you wanted to copy unique_ptr(s), wouldn't it be better to instead use a shared_ptr? What are the advantages of using a DeepPtr over a shared_ptr? – Ben J Sep 4 '15 at 12:34
  • 1
    @BenJ: He happens to be holding the state of the object in a pointer (because he is using PIMPL). When he copies the object he wants to copy the state for the new object. If you use shared pointer then you are using shared state (thus it is not a copy). Thus mutating would affect both objects. – Martin York Sep 4 '15 at 15:03
  • Self plug: I wrote a couple of blogs about writing smart pointers here: – Martin York Sep 4 '15 at 15:06
  • Shared pointers would shallow copy, but I want deep copy. – Matthew James Briggs Sep 4 '15 at 15:14
up vote 4 down vote accepted

Pragma(s)

Pragma once (like all pragmas) is not supported by all compilers.

#pragma once

Prefer to use header guards.
You can generate a GUID or you can use the namespace and file name to make it unique.

Namespace

You have not put your code into a namespace.

Constructor nullptr

There are some situations where it is nice to be able to construct with an explicit nullptr.
See here

    DeepPtr() :myPtr( nullptr ) {}

So you could modify this constructor slightly.

    DeepPtr(std::nullptr_t value = nullptr): myPtr(nullptr) {}

Move Construction.

I though you said on move you wanted to move the object not copy it.

    DeepPtr( DeepPtr&& other )
    :myPtr( nullptr )
    {
        if ( other )
        {
            myPtr = std::unique_ptr<T>{ new T{ *other } };
        }
    }

This does a copy on move construction. Also you should mark you move constructor as noexcept. This will give you several advantages when using the object with STL containers (the STL will default to copy even if move is available if the move is not not marked noexcept. This is to provide the strong exception grantee).

Note: You should only mark it as noexcept if it will not throw. So you should not do that if you are going to copy only; if you change this to do an actual move then add noexcept.

Copy and Swap

This is technically correct.

    DeepPtr& operator=( const DeepPtr& other )
    {
        DeepPtr temp{ other };
        swap( *this, temp );
        return *this;
    }

But you get a better optimization if you use passing by value to get your copy. Like this:

    DeepPtr& operator=(DeepPtr other)  // Pass by value gets copy
    {
        swap( *this, other );
        return *this;
    }

See What is the copy-and-swap idiom?

Move Assignment

Looks good. But like the move constructor, The move assignment should be marked as noexcept.

    DeepPtr& operator=( DeepPtr&& other )
    {
        swap( *this, other );
        return *this;
    }

Swap

    static void swap( DeepPtr& left, DeepPtr& right ) { std::swap( left.myPtr, right.myPtr ); }

Missed the static first time threw. It is more normal to define a normal swap method. Then define a standalone swap function. Also like move operations the swap should be marked as noexcept.

    void swap(DeepPtr& other) noexcept {
        using std::swap;
        swap(*this.myPtr, other.myPtr);

           // Adding `using std::swap`
           // Then calling swap (without std::)
           // Is the normal way of doing this. As it allows for Koenig
           // look up of a type specific swap first. If that does not
           // exist then the std::swap version will be used by the compiler.
    }
};
// outside the class
void swap(DeepPtr& lhs, DeepPtr& rhs) noexcept {
    lhs.swap(rhs);
}

Now that I have done swap. I would change the move operators too:

    DeepPtr(DeepPtr&& other)    noexcept
        :myPtr( nullptr )
    {
        other.swap(*this);
    }
    DeepPtr& operator=( DeepPtr&& other ) noexcept
    {
        other.swap(*this);
        return *this;
    }

Constness

    const T* const get() const { return myPtr.get(); }

Sure but are you only ever going to want a const version of the pointer. Like the last two operations operator* and operator-> you probably want a symmetric non const version.

Bool cast

You should mark this explicit.

    operator bool() const { return (bool)myPtr; }

See here https://codereview.stackexchange.com/a/93353/507

You already accepted Loki's answer (and he made good points) but there is one comment I wanted to add regarding this:

•Any other pitfalls that could be caused by this class.

Your class has no information on the concrete type held within, causing slicing in the presence of derived classes;

Consider this scenario:

class Base { ... }; // not abstract (can be instantiated)
class Derived: public Base { ... };

auto ptr1 = DeepPtr<Base>(new Derived{});
auto ptr2 = ptr1;

The last line will populate ptr2 with a Base instance that has all the sliced values from ptr2's Derived instance (because ptr2's constructorr calls new T, which resolves to new Base - not new Derived).

You have two possible solutions for this:

  • intrusive cloning (your class hierarchy rooted in Base needs to implement a virtual Base* clone() = 0 where each specialization of Base returns it's own instance); this cloning implementation would be then used for deep copy;

  • transparent cloning, based on template specialization of a factory function, by the derived type. I needed to implement something similar for the same reason (deep copy of polymorphic pointers). You can see here my implementation.

    See the use of to_polymorphic functions in my question, and how they instantiate the correct cloning function, transparently.

  • You can also store a Copier, like you can store a custom deleter in unique_ptr, to handle the copying. Construction needs to go through a make_deep_ptr which injects the proper copier without the use ever having to worry about it. – rubenvb Nov 3 '16 at 22:02

Edit: I'll leave this answer for completeness but you probably shouldn't inherit from a smart pointer or be very careful when doing so: is-it-ok-to-inherit-from-the-c11-smart-pointers-and-override-the-relative-oper

It seems to me like there's lot of code that simply wraps the functionality of the unique_ptr. Putting aside that having a copyable unique_ptr seems a bit dubious (although it seems like a reasonable thing in this context) why don't we inherit from unique_ptr and provide the copy operators?

template <typename T>
struct pimpl_ptr : public std::unique_ptr<T>
{
    using std::unique_ptr<T>::unique_ptr;
    pimpl_ptr() {};
    pimpl_ptr(pimpl_ptr<T> const& other)
    { 
        auto value = *other.get();
        this->reset(new T(value));
    }
    pimpl_ptr<T>& operator=(pimpl_ptr<T> const& other)
    {
        auto value = *other.get();
        this->reset(new T(value));
        return *this;
    }
};

There's probably a handful of improvements that could be made, as per Loki Astari's answer, but I think conceptually this will do the same but in less lines of code.

  • Hi and welcome to CodeReview. There's no need for this to be a comment, it is a perfectly valid and useful answer. – Kaz Sep 4 '15 at 17:24
  • Thank you Zak, I'll edit the comment out of the post since it's unnecessary. – user975326 Sep 4 '15 at 17:26
  • You may also want to come and say hi in chat – Kaz Sep 4 '15 at 17:28
  • 1
    Not sure if auto automaticlly becomes a reference or an object. So you way want to be explicit in using a reference: auto const& rather than auto – Martin York Sep 4 '15 at 17:44
  • I hadn't considered inheritance. Thank you for this. – Matthew James Briggs Sep 6 '15 at 15:30

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.