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;

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

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

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

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.