shared_ptr implementation

I've written a linked-list version of something like shared_ptr, which gets destroyed when the last copy of a pointer is destroyed.

Aside from the thread-unsafety, is it a fine implementation? Anything I could improve?

Also, what's the best way to extend it to make it work with HANDLEs, etc., without repeating (or limiting) myself unnecessarily, while avoiding excess verbosity (e.g. auto_ > is considered too verbose)?

template
class auto_
{
    T *pValue;
    mutable const auto_ *pPrev, *pNext;

public:
    auto_()           : pValue(new T()),  pPrev(NULL), pNext(NULL) { }
    auto_(T *pValue)  : pValue(pValue),   pPrev(NULL), pNext(NULL) { }
    auto_(const T &v) : pValue(new T(v)), pPrev(NULL), pNext(NULL) { }
    auto_(const auto_ &o) : pValue(o.pValue), pPrev(&o), pNext(NULL) { o.pNext = this; }

    virtual ~auto_()
    {
        const auto_ *const pPrev = this->pPrev, *const pNext = this->pNext;
        if (pPrev != NULL) { pPrev->pNext = pNext; }
        if (pNext != NULL) { pNext->pPrev = pPrev; }
        if (pPrev == NULL && pNext == NULL) { delete this->pValue; }
        this->pPrev = this->pNext = NULL;
        this->pValue = NULL;
    }

    auto_& operator=(const auto_& other)
    {
        if (this != &other)
        {
            this->~auto_();
            this->pValue = other.pValue;
            this->pPrev = &other;
            this->pNext = other.pNext;
            if (other.pNext != NULL) { other.pNext->pPrev = this; }
            other.pNext = this;
        }
        return *this;
    }

    operator   T&() /*also const version*/ { return *this->pValue; }
    operator   T*() /*also const version*/ { return  this->pValue; }
    T* operator->() /*also const version*/ { return  this->pValue; }
    T& operator *() /*also const version*/ { return *this->pValue; }
};

Sample usage:

```
template
T recurse(T value, int depth)
{
if (depth > 0) { T result = recurse(value, depth - 1); return result; }
else { return value; }
}

aut

Implementing your own smart pointer is very hard please don't try.

If it is just to try and learn then fine you may learn something but after practicing go back to one that has been tested and is know to work.

20 seconds into looking Bug 1:

int main()
{
    auto_   x;
    auto_   y(x);
    auto_   z(x);
}

Problem caused by this line:

auto_(const auto_ &o)
   : pValue(o.pValue)
   , pPrev(&o)
   , pNext(NULL)
{
    o.pNext = this;   // Here you are overwriting x.pNext when building z
}                     // It may or may not cause a bug but it was definitely
                      // not what you intended to do.

You now have

pNext List
[X] -> [Z] -> |    Y/Z point at nothing
       [Y] -> |    X   points at Z

pPrev List
[Y] -> [X] -> |    Y/Z point at X
[Z] ----^          X   points at nothing.

                   One assumes you are trying to create a circular list!

Don't use hungarian notation

pValue

Do you really want to bind your object to always using pointers?

Just use logical names for the members try not to encode type information into the name of the member it already has that information in its type.

Style Tip

Don;t do this:

const auto_ *const pPrev = this->pPrev, *const pNext = this->pNext;

It really hard to read. You are doing complex enough stuff try and make it easy to read

auto_ const* const pPrev = this->pPrev
    auto_ const* const pNext = this->pNext;

Don't do pointless work

this->pPrev = this->pNext = NULL;
    this->pValue = NULL;

This does nothing. This object is going to be destroyed. Therefore these variables do not exist after the destructor exists. So little point in playing with their values just before destruction.

Don't manually call the destructor

this->~auto_();

Nobody expects people to do this. Its just confusing. Move the code in the destructor into another method and call that.

Broken link

If sombody inserts a NULL pointer into your smart pointer you are asking for trouble when they de-reference it.

auto_  x(NULL); // valid constructor

This is going to blow up

operator   T&() /*also const version*/ { return  *this->pValue; }

You either need to check the ptr on construction (to make sure it is never NULL) or if you allow NULL pointers then you need to check when the object is used.

Specialization

To avoid repeating yourself allow your code to be specialized by a second template parameter that understands how to reclaim the resources of different types. The default version just calls delete but this allows you to use specialize it for handles etc.

Circular lists are easier with no NULL pointers.

template
struct Deleter
{
    void operator()(T* ptr) const { delete ptr;}
};

template >
class my_auto
{
    T *value;
    mutable const my_auto* prev;
    mutable const my_auto* next;

    public:
    // set up the chain to be circular pointing at just itself.
    // This way next/prev will never be NULL and we don't need to test
    my_auto()           : value(new T()),  prev(this), next(this) { }
    my_auto(T *value)   : value(value),    prev(this), next(this) { if (value == 0) throw int(1);}
    my_auto(const T &v) : value(new T(v)), prev(this), next(this) { }

    // insertInto() will do that appropriate set up.
    // We are currently not in a chain and have no value to release.
    my_auto(const my_auto &o)
    {
        insertInto(o);
    }

    my_auto& operator=(const my_auto& other)
    {
        // Check for assignment to self
        if (this != &other)
        {
            testAndDestroy();
            removeFromChain();
            insertInto(other);
        }
        return *this;
    }

    ~my_auto()
    {
        testAndDestroy();
        removeFromChain();
    }

    private:
        // If next == this then this is the only node in the chain
        // So we destroy the data portion.
        void testAndDestroy()
        {
            if (next == this)
            {
                D     deleter;
                deleter(value);
            }
        }
        // Unlink this node from the chain.
        // If we are the only link in the chain it still works
        // we just remain linked to ourselves.
        void removeFromChain()
        {
            // Remove this node from the chain
            prev->next  = next;
            next->prev  = prev;
        }
        // Insert into another chain.
        // Assumes that we are not part of another chain
        // and that our data has already been released as required.
        void insertInto(const my_auto& other)
        {
            value           = other.value;

            next            = other.next;
            other.next.prev = this;

            prev            = other;
            other.next      = this;
        }
};

If you want to use a non circular list (then you need to fix the constructor)

```
auto_(const auto_ &o)
: pValue(o.pValue)
, pPrev(&o)
, pNext(o.pNext)

StackExchange Code Review Q#4550, answer score: 12