patterncppModerate
Unique pointer implementation
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
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
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:
Here you are forcing an unnecessary copy:
Destructors should be written so they do not throw exceptions:
This can generate undefined behavior
This is broken and does not work as expected if _ptr is NULL
This should NEVER happen
If two unique ptrs point at the same object then you are well and truly going to get screwed. The whole point of
Comparing ptr via
When doing comparisons via
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.