Make_shared - own implementation

Question

I am trying to make my own implementation of shared_ptr. I have problems with make_shared. The main feature of std::make_shared that it allocates counter block and object in continuous block of memory. How I can do the same?

I tried do something like that:

template<class T>
class shared_ptr
{
private:
    class _ref_cntr
    {
    private:
        long counter;

    public:
        _ref_cntr() :
            counter(1)
        {
        }

        void inc()
        {
            ++counter;
        }

        void dec()
        {
            if (counter == 0)
            {
                throw std::logic_error("already zero");
            }

            --counter;
        }

        long use_count() const
        {
            return counter;
        }
    };

    template<class _T>
    struct _object_and_block
    {
        _T object;
        _ref_cntr cntr_block;

        template<class ... Args>
        _object_and_block(Args && ...args) :
            object(args...)
        {
        }
    };

    T* _obj_ptr;
    _ref_cntr* _ref_counter;

    void _check_delete_ptr()
    {
        if (_obj_ptr == nullptr)
        {
            return;
        }

        _ref_counter->dec();

        if (_ref_counter->use_count() == 0)
        {
            _delete_ptr();
        }

        _obj_ptr = nullptr;
        _ref_counter = nullptr;
    }

    void _delete_ptr()
    {
        delete _ref_counter;
        delete _obj_ptr;
    }

    template<class _T, class ... Args>
    friend shared_ptr<_T> make_shared(Args && ... args);

public:
    shared_ptr() :
        _obj_ptr(nullptr),
        _ref_counter(nullptr)
    {
    }

    template<class _T>
    explicit shared_ptr(_T* ptr)
    {
        _ref_counter = new counter_block();
        _obj_ptr = ptr;
    }

    template<class _T>
    shared_ptr(const shared_ptr<_T> & other)
    {
        *this = other;
    }

    template<class _T>
    shared_ptr<T> & operator=(const shared_ptr<_T> & other)
    {
        _obj_ptr = other._obj_ptr;
        _ref_counter = other._ref_counter;

        _ref_counter->inc();

        return *this;
    }

    ~shared_ptr()
    {
        _check_delete_ptr();
    }

};

template<class T, class ... Args>
shared_ptr<T> make_shared(Args && ... args)
{
    shared_ptr<T> ptr;
    auto tmp_object = new shared_ptr<T>::_object_and_block<T>(args...);
    ptr._obj_ptr = &tmp_object->object;
    ptr._ref_counter = &tmp_object->cntr_block;

    return ptr;
}

But when I delete object and counter block, the invalid heap block exception occurs.

Answer 1

N.B. _T is a reserved name and you must not use it for names of your own types/variables/parameters etc.

The problem is here:

void _delete_ptr()
{
    delete _ref_counter;
    delete _obj_ptr;
}

This is wrong for the make_shared case because you didn't allocate two separate objects.

The approach taken for make_shared in Boost's and GCC's shared_ptr is to use a new derived type of control block, which includes the reference counts in the base class and adds storage space for the managed object in the derived type. If you make _ref_cntr responsible for deleting the object via a virtual function then the derived type can override that virtual function to do something different (e.g. just use an explicit destructor call to destroy the object without freeing the storage).

If you give _ref_cntr a virtual destructor then delete _ref_counter will correctly destroy the derived type, so it should become something like:

void _delete_ptr()
{
    _ref_counter->dispose();
    delete _ref_counter;
}

Although if you don't plan to add weak_ptr support then there is no need to separate the destruction of the managed object and the control block, you can just have the control block's destructor do both:

void _delete_ptr()
{
    delete _ref_counter;
}

Your current design fails to support an important property of shared_ptr, which is that the template<class Y> explicit shared_ptr(Y* ptr) constructor must remember the original type of ptr and call delete on that, not on _obj_ptr (which has been converted to T*). See the note in the docs for the corresponding constructor of boost::shared_ptr. To make that work the _ref_cntr needs to use type-erasure to store the original pointer, separate from the _obj_ptr in the shared_ptr object, so that _ref_cntr::dispose() can delete the correct value. That change in the design is also needed to support the aliasing constructor.

class _ref_cntr
{
private:
    long counter;

public:
    _ref_cntr() :
        counter(1)
    {
    }

    virtual ~_ref_cntr() { dispose(); }

    void inc()
    {
        ++counter;
    }

    void dec()
    {
        if (counter == 0)
        {
            throw std::logic_error("already zero");
        }

        --counter;
    }

    long use_count() const
    {
        return counter;
    }

    virtual void dispose() = 0;
};

template<class Y>
struct _ptr_and_block : _ref_cntr
{
    Y* _ptr;
    explicit _ptr_and_block(Y* p) : _ptr(p) { }
    virtual void dispose() { delete _ptr; }
};

template<class Y>
struct _object_and_block : _ref_cntr
{
    Y object;

    template<class ... Args>
    _object_and_block(Args && ...args) :
        object(args...)
    {
    }

    virtual void dispose() { /* no-op */ }
};

With this design, make_shared becomes:

template<class T, class ... Args>
shared_ptr<T> make_shared(Args && ... args)
{
    shared_ptr<T> ptr;
    auto tmp_object = new shared_ptr<T>::_object_and_block<T>(args...);
    ptr._obj_ptr = &tmp_object->object;
    ptr._ref_counter = tmp_object;

    return ptr;
}

So _ref_counter points to the allocated control block and when you do delete _ref_counter that means you you have a correctly-matched new/delete pair that allocates and deallocates the same object, instead of creating one object with new then trying to delete two different objects.

To add weak_ptr support you need to add a second count to the control block, and move the call to dispose() out of the destructor, so it is called when the first count goes to zero (e.g. in dec()) and only call the destructor when the second count goes to zero. Then to do all that in a thread-safe way adds a lot of subtle complexity that would take much longer to explain than this answer.

Also, this part of your implementation is wrong and leaks memory:

void _check_delete_ptr()
{
    if (_obj_ptr == nullptr)
    {
        return;
    }

It's possible to constructor a shared_ptr with a null pointer, e.g. shared_ptr<int>((int*)nullptr), in which case the constructor will allocate a control block, but because _obj_ptr is null you will never delete the control block.