Writing a copyable unique_ptr for copyable classes with PIMPL idiom

user_14471855 0
Viewed 135

I want to write a class that uses the PIMPL idiom but makes the following possible:

int main() {
    MyClass myclass {12};
    std::string val = myclass.get_value();
    std::cout << val << "\n";

    MyClass copy = myclass;
    val = copy.get_value();

    return 0;
}

That is, provide copy functionality while using PIMPL. Using std::unique_ptr to implement PIMPL leads to the expected compile error:

error C2280: 'MyClass::MyClass(const MyClass &)': attempting to reference a deleted function

This is my attempt:

class.hpp

#pragma once

#include <string>

#include "copyable_unique_ptr.hpp"

class MyClass {
private:
    struct MyClassPrivate;
    copyable_unique_ptr<MyClassPrivate> _impl;

public:
    MyClass(int value);
    ~MyClass();

    std::string get_value();
};

class.cpp

#include "class.hpp"

#include <string>

struct MyClass::MyClassPrivate {
    int value;
};

MyClass::MyClass(int value) 
    : _impl(std::make_unique<MyClassPrivate>()) {
    _impl->value = value;
}

MyClass::~MyClass() = default;

std::string MyClass::get_value() {
    return std::to_string(_impl->value);
}

copyable_unique_ptr.hpp

#pragma once

#include <memory>

template <typename T>
class copyable_unique_ptr {
private:
    std::unique_ptr<T> _ptr;

public:
    copyable_unique_ptr(std::unique_ptr<T> &&ptr)
        : _ptr(std::move(ptr)) {}

    ~copyable_unique_ptr() = default;

    copyable_unique_ptr(const copyable_unique_ptr<T> &other) {
        _ptr = std::make_unique<T>(*other._ptr);
    }

    copyable_unique_ptr(copyable_unique_ptr<T> &&other) {
         _ptr = std::move(other._ptr);
    }

    copyable_unique_ptr &operator=(const copyable_unique_ptr<T> &other) {
        _ptr = std::make_unique<T>(*other._ptr);
    }

    copyable_unique_ptr &operator=(copyable_unique_ptr<T> &&other) {
         _ptr = std::move(other._ptr);
    }

    T *operator->() {
        return _ptr.get();
    }
};

which leads to the following error: warning C4150: deletion of pointer to incomplete type 'MyClass::MyClassPrivate'; no destructor called

I also had this error when first trying to use unique_ptr, and the solution was to move the destructor into class.cpp: MyClass::~MyClass() = default;.

I don't really know how to solve this problem while using the approach with copyable_unique_ptr.

I saw this question on SO. They are trying to do the same as me, but the last comment to the answer suggests that they are having exactly the same problem.

c++11 c++
3 Answers

Although you can write a custom smart pointer here, I always like to reuse as much code as possible. It seems you want a dynamically created object with copy semantics rather than pointer semantics. For that reason I would hesitate to call your copyable_unique_ptr a pointer at all. I did write a class for this once and settled on something like dynamic_object<T>.

Instead of a pointer I would do this using a std::vector that contains just a single element. Something a bit like this:

class MyClass {
private:
    struct MyClassPrivate;
    std::vector<MyClassPrivate> _impl;

public:
    MyClass(int value);
    ~MyClass();

    std::string get_value();
};

struct MyClass::MyClassPrivate {
    int value;
};

MyClass::MyClass(int value)
    : _impl(1) {
    _impl.front().value = value;
}

MyClass::~MyClass() = default;

std::string MyClass::get_value() {
    return std::to_string(_impl.front().value);
}

However if you really want to create your copyable_unique_ptr I woud still use a std::vector here to simplify its codeing and to reuse high quality Standard Library code:

Perhaps a bit like this:

template<typename T>
class copyable_unique_ptr
: private std::vector<T>
{
    using base_type = std::vector<T>;

public:
    copyable_unique_ptr(std::unique_ptr<T>&& ptr)
    : base_type(std::move(*ptr), 1) {}

    copyable_unique_ptr(std::unique_ptr<T> const& ptr)
    : base_type(*ptr, 1) {}

    T& operator*()  { return base_type::back(); }
    T* operator->() { return base_type::data(); }

    T const& operator*()  const { return base_type::back(); }
    T const* operator->() const { return base_type::data(); }
};
user_38077290

Sam Varshavchik gave a hint in the comment that also moving copy constructor and assignment operator to the implementation fixes the code that I was trying to write, that means:

class.hpp

#pragma once

#include <string>

#include "copyable_unique_ptr.hpp"

class MyClass {
private:
    struct MyClassPrivate;
    copyable_unique_ptr<MyClassPrivate> _impl;

public:
    MyClass(int value);

    MyClass(const MyClass &);
    MyClass(MyClass &&);
    MyClass &operator=(const MyClass &);
    MyClass &operator=(MyClass &&);
    ~MyClass();

    std::string get_value();
};

class.cpp

#include "class.hpp"

#include <string>
#include <iostream>

struct MyClass::MyClassPrivate {
    int value;
};

MyClass::MyClass(int value)
    : _impl(std::make_unique<MyClassPrivate>()) {
    _impl->value = value;
}

MyClass::MyClass(const MyClass &) = default;
MyClass::MyClass(MyClass &&) = default;
MyClass &MyClass::operator=(const MyClass &) = default;
MyClass &MyClass::operator=(MyClass &&) = default;
MyClass::~MyClass() = default;


std::string MyClass::get_value() {
    std::cout << &_impl << "\n";
    return std::to_string(_impl->value);
}

And it works as expected.

On the other hand, Galik gave an answer that gives a much better and less complex alternative to copyable_unique_ptr. I will prefer using his way.

user_144718550

Its probably simpler to stick to a normal unique_ptr and just implement copying of MyClass in your cpp file:

class MyClass {
private:
    struct MyClassPrivate;
    std::unique_ptr<MyClassPrivate> _impl;

public:
    MyClass(int value);
    MyClass(MyClass&&);
    MyClass(const MyClass&);
    ~MyClass();

    MyClass& operator = (const MyClass&);
    MyClass& operator = (MyClass&&);

    std::string get_value();
};

MyClass::MyClass(const MyClass& other)
    : _impl(std::make_unique<MyClassPrivate>(*other._impl)) {
}

MyClass& MyClass::operator = (const MyClass& other) {
    _impl = std::make_unique<MyClassPrivate>(*other._impl);
    return *this;
}

MyClass::~MyClass() = default;
MyClass::MyClass(MyClass&&) = default;
MyClass& MyClass::operator = (MyClass&&) = default;

There is a slight issue with this code (and your original code) in that the moved from objects will have a null _impl you might want to add checks for null, alternatively you could replace with a default constructed object instead of a null pointer but that might be a waste in most cases?

user_54943700
Related