HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Unique pointer implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
uniqueimplementationpointer

Problem

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
  struct deleter
  {
    static void release(T *p) 
    {
      if (p) {
        delete p;
        p = 0;
      }
    }
  };

  template
  struct array_deleter
  {
    static void release(T *p)
    {
      if (p) {
        delete [] p;
        p = 0;
      }
    }
  };

  template  >
  class unique_ptr
  {
  private:
    T *_ptr;

    template  unique_ptr(unique_ptr &);
    template  unique_ptr &operator=(unique_ptr &);

  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 
    unique_ptr &operator=(unique_ptr 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
unique_ptr::unique_ptr() :
_ptr(0)
{}

template
unique_ptr::unique_ptr(T *p) :
_ptr(p)
{}

template
unique_ptr::~unique_ptr()
{
reset();
}

template
unique_ptr &unique_ptr::operator=(unique_ptr u)
{
reset(u.release());
return *this;
}

template
template
unique_ptr &unique_ptr::operator=(unique_ptr u)
{
reset(u.release());
return *this;
}

template
T unique_ptr::operator*() const
{
return *_ptr;
}

template
T *unique_ptr::operator->() const
{
return _ptr;
}

template
T *unique_p

Solution

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 
unique_ptr &unique_ptr::operator=(unique_ptr 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 
unique_ptr::~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 
T unique_ptr::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 
  void unique_ptr::reset(T *p = 0)
  {
    if (_ptr != p) {
      if (_ptr) {
        unique_ptr::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 &x, const unique_ptr &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).

Code Snippets

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

Context

StackExchange Code Review Q#29629, answer score: 12

Revisions (0)

No revisions yet.