I wrote a class to implement auto_ptr

Question

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

Answer 1

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

using namespace std;

See: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

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

Answer 2

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