Unique pointer implementation

Question

The interface and some of the implementation is take from Boost. However, it is intended to be transferable between projects by only copying one file. I have removed most of the policy classes in favor of more condensed code that requires less files to move around. Please tell me where my strengths are as well as where the code might fall apart.

unique_ptr.h -- Interface and Some Implementation

#ifndef UNIQUE_PTR_HPP_INCLUDED
#define UNIQUE_PTR_HPP_INCLUDED
namespace glext
{
 template<class T>
 struct deleter
 {
 static void release(T *p) 
 {
 if (p) {
 delete p;
 p = 0;
 }
 }
 };
 template<class T>
 struct array_deleter
 {
 static void release(T *p)
 {
 if (p) {
 delete [] p;
 p = 0;
 }
 }
 };
 template <class T, class D = glext::deleter<T> >
 class unique_ptr
 {
 private:
 T *_ptr;
 template <class U, class D> unique_ptr(unique_ptr<U, D> &);
 template <class U, class D> unique_ptr &operator=(unique_ptr<U, D> &);
 public:
 typedef T element_type;
 typedef D deleter_type;
 unique_ptr();
 explicit unique_ptr(T *p);
 ~unique_ptr();
 unique_ptr &operator=(unique_ptr u);
 template <class U>
 unique_ptr &operator=(unique_ptr<U, D> u);
 T operator*() const;
 T *operator->() const;
 T *get() const;
 T *release();
 void reset(T *p = 0);
 void swap(unique_ptr &u);
 };
}
#include "unique_ptr.inl"

unique_ptr.inl -- Implementation

namespace glext
{
 template <class T, class D>
 unique_ptr<T, D>::unique_ptr() :
 _ptr(0)
 {}
 template <class T, class D>
 unique_ptr<T, D>::unique_ptr(T *p) :
 _ptr(p)
 {}
 template <class T, class D>
 unique_ptr<T, D>::~unique_ptr()
 { 
 reset();
 }
 template <class T, class D>
 unique_ptr<T, D> &unique_ptr<T, D>::operator=(unique_ptr<T, D> u)
 {
 reset(u.release());
 return *this;
 }
 template <class T, class D>
 template <class U>
 unique_ptr<T, D> &unique_ptr<T, D>::operator=(unique_ptr<U, D> u)
 {
 reset(u.release());
 return *this;
 }
 template <class T, class D>
 T unique_ptr<T, D>::operator*() const
 {
 return *_ptr;
 }
 template <class T, class D>
 T *unique_ptr<T, D>::operator->() const
 {
 return _ptr;
 }
 template <class T, class D>
 T *unique_ptr<T, D>::get() const
 {
 return *_ptr; 
 }
 template <class T, class D>
 T *unique_ptr<T, D>::release()
 {
 T *tmp = _ptr;
 _ptr = 0;
 return tmp;
 }
 template <class T, class D>
 void unique_ptr<T, D>::reset(T *p = 0)
 {
 if (_ptr != p) {
 if (_ptr) {
 unique_ptr<T, D>::deleter_type::release(_ptr);
 _ptr = p;
 }
 }
 }
 template <class T, class D>
 void swap(unique_ptr<T, D> &u)
 {
 std::swap(_ptr., u._ptr);
 }
 template <class T, class D>
 inline void swap(unique_ptr<T, D> &x, unique_ptr<T, D> &y)
 {
 x.swap(y);
 }
 template <class T1, class D1, class T2, class D2>
 bool operator==(const unique_ptr<T1, D1> &x, const unique_ptr<T2, D2> &y)
 {
 return x.get() == y.get();
 }
 template <class T1, class D1, class T2, class D2>
 bool operator!=(const unique_ptr<T1, D1> &x, const unique_ptr<T2, D2> &y)
 {
 return x.get() != y.get();
 }
 template <class T1, class D1, class T2, class D2>
 bool operator<(const unique_ptr<T1, D1> &x, const unique_ptr<T2, D2> &y)
 {
 return x.get() < y.get();
 }
 template <class T1, class D1, class T2, class D2>
 bool operator<=(const unique_ptr<T1, D1> &x, const unique_ptr<T2, D2> &y)
 {
 return x.get() <= y.get();
 }
 template <class T1, class D1, class T2, class D2>
 bool operator>(const unique_ptr<T1, D1> &x, const unique_ptr<T2, D2> &y)
 {
 return x.get() > y.get();
 }
 template <class T1, class D1, class T2, class D2>
 bool operator>=(const unique_ptr<T1, D1> &x, const unique_ptr<T2, D2> &y)
 {
 return x.get() >= y.get();
 }
}

Sample usage

int main(int /*argc*/, char * /*argv*/[]) {
 alloc_console();
 glext::unique_ptr<int> uptr(new int(20));
 glext::unique_ptr<int, glext::array_deleter<int> > uaptr(new int[20]);
}

• Loki Astari: This is good for an excercise. But please do NOT do this for production code. There are just too many gotchs involved in writing a correct smart pointer. Check out Scott Myers; he has talked at length about his own personal attempt that was still not working in all situations after many years of code reviews and feedback from lots of very experienced people. Use the standard ones provided by the standard. They are highly tested.
• Matthew Hoggan: @Loki Astari Point well taken, but C++ 99 only provides auto_ptr and most companies I have worked for still are not using C++-11. With that being said, I know that boost provides one which should handle most uses, but what if you want a less complex one to port from project to project without the overhead of coppying every necessary header boost requires to get the code to build? This is what I am interested in.
• Loki Astari: The use std::auto_ptr. Its not exactly that bad. And its better than what you have proposed (see my comments below). The only reason people don't like std::auto_ptr is the hidden move semantics on assignment (that apparently is not intuitive) which are fixed by std::move with the new std::unique_ptr. Why not install boost when you install the compiler. Its not as if that is uncommon. Why would you want to copy the header files around. You don't even need to build boost (the smart pointers are part of the header only libraries).
• Matthew Hoggan: Your comment on using auto_ptr is fine and all except gcc states "The resulting rule is simple: Never ever use a container of auto_ptr objects. The standard says that "undefined" behavior is the result, but it is guaranteed to be messy." Which comes from gcc.gnu.org/onlinedocs/libstdc++/manual/auto_ptr.html
• Jerry Coffin: Just for what it's worth: there's a C++98 and a C++03, but no C++99.

Answer 1

Lets start here:

static void release(T *p) 
{
 if (p) { // No need to check for NULL
 // delete has no action when applied to a NULL pointer
 delete p;
 p = 0; // This is very dangerous.
 // It has no actual affect (as it is local)
 // but provides an illusionary sense of security.
 }
}

Here you are forcing an unnecessary copy:

template <class T, class D>
unique_ptr<T, D> &unique_ptr<T, D>::operator=(unique_ptr<T, D> u)
 // ^^^^^^^^^^^^^^^^^^
 // Pass by value forcing a copy
 // Why are you doing that.
 // If you have a reason it should be
 // documented.
{
 reset(u.release());
 return *this;
}

Destructors should be written so they do not throw exceptions:

template <class T, class D>
unique_ptr<T, D>::~unique_ptr()
{ 
 reset(); // This calls delete which calls the destructor of T
 // Since you have no control over the type T you should take
 // precautions over what happens next.
}

This can generate undefined behavior

template <class T, class D>
T unique_ptr<T, D>::operator*() const
{
 return *_ptr; // _ptr is NULL then this is UB
 // Is this really what you want. If so then it should
 // be explicitly documented.
}

This is broken and does not work as expected if _ptr is NULL

 template <class T, class D>
 void unique_ptr<T, D>::reset(T *p = 0)
 {
 if (_ptr != p) {
 if (_ptr) {
 unique_ptr<T, D>::deleter_type::release(_ptr);
 _ptr = p; // You are only assigning to _ptr if it is NOT NULL
 // Thus if _ptr is NULL you are leaking the `p`
 // Also most smart pointers gurantee that once you have
 // passed a pointer you take ownership and delete it.
 // If the above call to release() throws an exception you
 // are again leaking `p`. You must put in extra code to 
 // make sure `p` is either deleted or assigned to `_ptr`
 // not matter what happens (even an exception).
 }
 }
 }

This should NEVER happen

bool operator==(const unique_ptr<T1, D1> &x, const unique_ptr<T2, D2> &y)
{
 return x.get() == y.get();
}

If two unique ptrs point at the same object then you are well and truly going to get screwed. The whole point of unique ptr is that they are unique.

Comparing ptr via operator < is a fool's errand. Unless both pointers are in the same block of memory (ie they were allocated via the same new) they are not comparable the results are otherwise undefined.

When doing comparisons via unique ptr you should be using the underlying object (NOT the pointers).

Answer 2

Firstly, let's fix up some errors that exist in the code.

1. You're missing an #endif at the end of your unique_ptr.h file.
2. Both template <class U, class D> unique_ptr(unique_ptr<U, D> &); and template <class U, class D> unique_ptr &operator=(unique_ptr<U, D> &); don't require template redefinitions. They should both just be unique_ptr(unique_ptr &); and unique_ptr& operator=(unique_ptr&);.
3. You need an #include <algorithm> to use std::swap. You probably also want to change it to using std::swap within your swap function. It should be unique_ptr<T, D>::swap, not swap. You've also got an extra . after _ptr in this function.
4. In your reset function in the .inl file, it's an error to redeclare the default parameter; that is, it should be template <class T, class D> void unique_ptr<T, D>::reset(T *p) (without the = 0).

Some general comments:

if (p) {
 delete p;
 p = 0;
}

The if is unnecessary. delete (and delete[]) already do a NULL check, and are a no-op if their argument is NULL.

Variables starting with _ are reserved for compiler usage. Switch to appending at the end of the variable name (ptr_) instead.

Your T operator*() const; should be returning by T& instead.

Your get function is trying to return a dereferenced pointer:

template <class T, class D>
T *unique_ptr<T, D>::get() const
{
 return *_ptr; // Should be return _ptr;
}

There are likely other things I've missed. Also, it's worth pointing out that your use cases will be severely constrained without move semantics and move aware containers.

• Matthew Hoggan: What I really want to know is how is the lack of move semantics going to affect me?
• Yuushi: For a really simple example, try creating a std::vector<glext::unique_ptr<T>> and see what happens.
• Matthew Hoggan: Aye I see that. That was an error forgot the copy constructor for unique_ptr<T, D>. unique_ptr<U, D> is not allowed according to boost, but one for unique_ptr<T, D> is with the usage of complex move semantics. I put in the copy constructor and used it with std::vector. All news were matched with delete on the same addresses. _CrtDumpMemoryLeaks() shows memory leaks in MSVC, but stepping through the code reveals to me there is none see above. What else am I missing?
• Yuushi: Your code, even with the fixes I talked about above, still doesn't compile for me.
• Matthew Hoggan: Hmm perhaps a bad copy and paste here. Code can be obtained at github.com/mehoggan/GLExtensions/tree/master/smart_ptrs/… and the header with contains all the std headers I need is found at github.com/mehoggan/GLExtensions/blob/master under glext.h.