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;
 }
}

• Edward: My review would likely be "Use std::shared_ptr instead."
• ZNone: I did it for practice only.
• Jamal: As @utnapistim as mentioned, you could also do your own tests and how them here. That will help determine how effective your implementation is in certain scenarios.

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.

• utnapistim: "Why counter is allocated dynamically?" - I would guess it is because the instance of the counter is shared, between all SharedPointer instances that refer to the same pointer.
• edmz: Indeed. That's what a static member variable is for.
• utnapistim: a static member variable would not work: for different pointer values, you need different cunters; for same pointer value, you need same counter. Implementing with a static counter, would break the first condition.
• edmz: What do you mean? The counter variable doesn't depend on T.
• utnapistim: @black, if you have two instances of SharedPointer <int> with a static counter, the counter will be 2 after creating them (instead of having each instance with a count of one). This means that all instances (except the last one to go out of scope - which sets the common count to zero), will be leaked on deletion.

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);
}