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

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.