# I wrote a class to implement auto_ptr

Help me review this code:

#include <iostream>
#include <cassert>
using namespace std;

template <typename T>
class auto_ptr
{
public:
explicit auto_ptr(T* p = NULL):m_p(p){}
auto_ptr(auto_ptr& other);
auto_ptr& operator=(auto_ptr& other);
T* get() const { return m_p; }
void reset(T* p = NULL);
T& operator * (){ return *m_p; }
T* operator -> (){ return m_p; }
~auto_ptr()
{
if(m_p)
delete m_p;
m_p = NULL;
}
private:
T* m_p;
T* release();
};

int main()
{
auto_ptr<int> p(new int(3));
cout<<*p<<endl;

int *b = new int(6);
p.reset(b);
cout<<*p.get()<<endl;

auto_ptr<int> p1(new int(10));
p = p1;
cout<<*p<<endl;
assert(NULL == p1.get());

auto_ptr<int> p2(new int(100));
auto_ptr<int>p3(p2);
cout<<*p3<<endl;
assert(NULL == p2.get());
}

template <typename T>
T* auto_ptr<T>::release()
{
T* temp = this->m_p;
this->m_p = NULL;
return temp;
}

template <typename T>
auto_ptr<T>::auto_ptr(auto_ptr& other)
{
m_p = other.release();
}

template <typename T>
auto_ptr<T>& auto_ptr<T>::operator=(auto_ptr& other)
{
if(this == &other)
return *this;
delete m_p;
m_p = other.release();
return *this;
}

template <typename T>
void auto_ptr<T>::reset(T* p)
{
if(m_p != p)
{
delete m_p;
m_p = p;
}
}


Assuming you are going to put this in a header file don't do thus

using namespace std;


These should be const as they don't change the state of your auto_ptr

T& operator * (){ return *m_p; }
T* operator -> (){ return m_p; }


The destructor

~auto_ptr()
{
if(m_p)            // No need to check for NULL
// delete on a NULL pointer is valid.
delete m_p;

m_p = NULL;        // This is pointless.
// The object is about to leave scope.
}


Why the sudden use of this!

template <typename T>
T* auto_ptr<T>::release()
{
T* temp = this->m_p;
//  ^^^^^^
this->m_p = NULL;
// ^^^^^^
return temp;
}


Assignment not exception safe.

template <typename T>
auto_ptr<T>& auto_ptr<T>::operator=(auto_ptr& other)
{
if(this == &other)
return *this;

// This is not exception safe.
// T is completely unknown to you.
// Therefore you must assume the person who wrote T is an idiot
// and therefore may throw an exception from there destructor.
delete m_p;

// If the above statement throws then your object is now in an undefined
// state with m_p pointing at an invalid object.

m_p = other.release();
return *this;
}


The correct way to do this.

template <typename T>
auto_ptr<T>& auto_ptr<T>::operator=(auto_ptr& other)
{
if(this == &other)
return *this;

auto_ptr<T> tmp = release();
m_p = other.release();

// After the state of your (and other objects)
// are completely defined and are in a good state.
// then you can tidy up any left over objects.

return *this;

// the destructor of tmp should now release it properly.
}


I presume you are aware of all the standard issues with auto_ptr, and that C++11 fixes by far most of them. I'm thus going to assume you have no C++11 support; if that's not the case, most of this doesn't apply because the whole thing should be rewritten with different semantics.

Comments in order I see things:

• You've combined everything into one file, so it may not be an issue in your code, but you should not have using-directives like using namespace std; at global scope in a header; that's rude to anyone who includes it.
• Your operator* and operator-> should both be const.
• Your destructor checks for m_p being NULL. This is unnecessary; delete is a no-op on a null pointer. Personally, I'd have the destructor simply call reset.
• Your class is missing operator bool (or operator void*), operator!, operator<<, swap, and any support for casting.
• You use this->m_p about half the time, and the other half use just m_p. Both are fine, but be consistent.
• Use c-tor initializers where possible, such as in the copy-constructor.

This is an okay base, but you're missing a great number of features that I would expect a smart pointer to have now: in particular, you don't support custom deleters or array types.

Also, on a purely stylistic choice: I would define the member functions of such a simple class template inline. You can't hide the implementation anyway, and when the overhead is so small I don't expect it to matter for readability. (I presume that for getting a list of functions you use your IDE's features, or something like Doxygen.)

• Its not only rude but can break other code by the inclusion of the header file. – Martin York Aug 14 '13 at 16:19