Simple shared pointer

Question

I wrote a simple shared pointer, which I think works pretty well. I would like to see your review of it.

This is the header file:

#pragma once

#include <algorithm>

template <class T>
class SharedPointer
{
private:
    T* ptr;
    int* counter;

    void swapp(SharedPointer& first, SharedPointer& second);

public:
    SharedPointer(void);
    SharedPointer(T* val);
    SharedPointer(const SharedPointer& sp);

    T& operator*();
    T* operator->();
    SharedPointer& operator=(const SharedPointer& sp);
    ~SharedPointer(void);
};

template <class T>
void SharedPointer<T>::swapp(SharedPointer& first, SharedPointer& second)
{
    using std::swap;

    swap(first.ptr, second.ptr);
    swap(first.counter, second.counter);
}

template <class T>
SharedPointer<T>::SharedPointer(void)
{
    ptr = new T;
    counter = new int;
    *counter = 1;
}

template <class T>
SharedPointer<T>::SharedPointer(T* val) : ptr(val)
{
    counter = new int;
    *counter = 1;
}

template <class T>
SharedPointer<T>::SharedPointer(const SharedPointer& sp)
{
    ptr = sp.ptr;
    counter = sp.counter;
    ++(*counter);
}

template <typename T>
T& SharedPointer<T>::operator*()
{
    return *ptr;
}

template <typename T>
T* SharedPointer<T>::operator->()
{
    return ptr;
}

template <class T>
SharedPointer<T>& SharedPointer<T>::operator=(const SharedPointer& sp)
{
    SharedPointer temp(sp);
    swapp(*this,temp);

    return *this;
}

template <class T>
SharedPointer<T>::~SharedPointer(void)
{
    if(--(*counter) == 0)
    {
        delete ptr;
        delete counter;
    }
}

Answer 1

Foreword

When developing your own version of some standard library or tool, I suggest you to specify how close to the standard version is along with the purpose behind it.
So, for istance:

• Is it standard compliant?
• Does it address anything missing?

---

Interface

What I find missing:

• How can I reset the pointer?
• How can I know how many "visitors" there are(that is, how do I get counter)? Note: It could be useful, but don't go too far and break the encapsulation!
• An efficient way to visit the object, that is an alternative to std::weak_ptr:
• How do I allocate/deallocate particular objects?

---

Implementation

• #pragma once is not standard. Although almost all compilers implement it and seems like to be faster than header guards, I recommend these last ones at least until it gets standardized.
• Why counter is allocated dynamically?
• I understand you may not name swapp swap; however, there are better names, such as:
  • m_swap
  • internal_swap
• C++, unlike C, makes no difference between function() and function(void). Yes, they're both correct; but the first one is much more common.
• new can throw std::bad_alloc. Especially in the second constructor, if new threw when allocating/constructing counter, ptr would be leaked.

Answer 2

Here are a few notes (not an exhaustive list):

The semantics of your shared pointer are a bit weird.

How will you assign an already created pointer to an already created SharedPointer instance?

What I mean:

SharedPointer<int> a{ new int{10} }; // allocate new int
SharedPointer<int> b; // I would expect this to create null, but fine
int *c = new int{20};
b = c; // operator= or SharedPointer<int>::reset, or something, is missing

SharedPointer<T>::swapp is redundant. You can rely on std::swap instead (as it uses move).

You should implement move semantics for your class.

ptr and the counter have the same lifetime. According to RAII and SRP principles, they should be in their own class (e.g. SharedPointer<T>::Data<T> should contain them, not the shared pointer class directly).

You cannot support custom deleters, which means you cannot pass them accross library boundaries on Windows (and probably on some other systems).

By looking at the interface, I would expect SharedPointer(void); to create a null SharedPointer, not a pointer to a default T instance.

Your class doesn't support custom allocators or this scenario:

SharedPointer<char> x{ new char[100] }; // exhibbits UB on scope end

Answer 3

Missing Featres

• release()
  There is no way to give up ownership.
• reset()
  You have to work around using Constructor and assignment. But this is complicated by not working with temporaries (no move semantics).
• Move Semantics
  Both constructor and assignment versions would be nice.
• No way to break cycles (Weak Pointer).
• swap()
• No relational operators defined.
  So you can not put it in a map/set.

Code Review

You can initialize POD data on creation with new.

int*   x = new int(5);   // creates an int and initializes
                         // it with the value of 5

You should prefer to use initializer lists rather than write code in the body of the constructor.

template <class T>
SharedPointer<T>::SharedPointer()  // don't use void :-( as a parameter.
    : ptr(new T())
    , counter(new int(1))
{}

As mentioned by other people I don't like the default initializer creating an object. Why not set it to null and have a count of 0? But your design you can also make an argument for the way you are doing it.

In the assignment operator you use the "Copy and Swap Idiom". But you use an atypical version. It is more common to use a pass by value version (you silently get an implicit copy). This version (apparently) has better opportunities for optimization. In depth description: What is the copy-and-swap idiom?

template <class T>
SharedPointer<T>& SharedPointer<T>::operator=(SharedPointer sp)
                                               // Notice pass by value.
{
    // Because we already have a copy.
    // Just do the swap with the parameter (it will die at the end of fucntion
    // and be correctly destroyed).
    swapp(*this, sp);
    return *this;
}

Why is your swap written like that?

If you write it like this:

void swapp(SharedPointer& first, SharedPointer& second);

Then you never touch the current object (you manipulate first/second) so it may as well be a static member of the class. It is more normal to just pass the second parameter (the first being the implicit this).

template <class T>
void SharedPointer<T>::swap(SharedPointer& rhs) noexcept
                                         //     ^^^^^^^^^
      // Don't forget that swap is supposed to always be exception safe.
{
    using std::swap;

    swap(this->ptr,     rhs.ptr);
    swap(this->counter, rhs.counter);
}

// A version of swap in the same namespace as your class.
// This allows for Kerning look-up (ADNL) to correctly activate.
template<typename T>
void swap(SharedPointer<T>& lhs, SharedPointer<T>& rhs)
{
    lhs.swap(rhs);
}