Return to Revisions

##Overall

Ohhhhh.

##Code Review

###Constructing from object!

    unique_ptr(T& t) {
       _ptr = &t;
    }

That's exceedingly dangerous.

{
    int x;
    unique_ptr<int>  data(x);
}
// Here the unique ptr calls delete on an automatic variable.

###Use member initializing list.

You should always attempt to use the member initializer list for initializing members. Any non trivial object will have its constructor called before the initializer code is called and thus it is inefficient to then re-initialize it in the code.

    unique_ptr(unique_ptr<T>&& uptr)
        : _ptr(std::move(uptr.ptr))
    {
       uptr._ptr = nullptr;
    }

###Member variable Names

Prefer not to use _ as the first character in an identifier name.

T* _ptr;  // Technically OK.

Even if you know all the rules of when to use them most people don't so they are best avoided. If you must have a prefix to identify members use m_ but if you name your member variables well then there is no need for any prefix (in my opinion prefixes makes the code worse not better, because you are relying on un-written rules. If you have good well defined names (see self documenting code) then members should be obvious).

###NoExcept

The move operators should be marked as noexcept.

When used with standard containers this will enable certain optimizations. This is because if the move is noexcept then certain operations can be guaranteed to work and thus provide the strong exception guarantee.

    unique_ptr(unique_ptr<T>&& uptr) noexcept
                              //     ^^^^^^^^

    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) noexcept
                                            //     ^^^^^^^^

###Leak in assignment

Note: Your current assignment potentially leaks. If this currently has a pointer assigned then you overwrite it without freeing.

    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) {
       if (this == uptr) return *this;

       // Here you override _ptr
       // But if it has a value then you have just leaked it.
       _ptr = std::move(uptr._ptr);

       uptr._ptr = nullptr;
       return *this;
    }

###Checking for this pesimization

    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) {
       if (this == uptr) return *this;
       _ptr = std::move(uptr._ptr);
       uptr._ptr = nullptr;
       return *this;
    }

Yes you do need to make it work when there is self assignment. But in real code the self assignment happens so infrequently that this test becomes a pesimization on the normal case (same applies for copy operation). There have been studies on this (please somebody post a link I have lost mine and would like to add it back to my notes).

The standard way of implementing move is via swap. Just like Copy is normally implemented by Copy and Swap.

    unique_ptr(unique_ptr<T>&& uptr) noexcept
        : _ptr(nullptr)
    {
        this->swap(uptr);
    }
    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) noexcept
    {
        this->swap(uptr);
        return *this;
    }
    void swap(unique_ptr<T>& other) noexcept
    {
        using std::swap;
        swap(_ptr, other._ptr);
    }

Using the swap technique also delays the calling of the destructor on the pointer for the current object. Which means that it can potentially be re-used. But if it is going out of scope the unique_ptr destructor will correctly destroy it.

###Summary Good first try but still lots of issues.

Please read the article I wrote on unique_ptr and shared pointer for lots more things you should implement.

Smart-Pointer - Unique Pointer
Smart-Pointer - Shared Pointer
Smart-Pointer - Constructors

Some things you missed:

• Constructor with nullptr
• Constructor from derived type
• Casting to bool
• Checking for empty
• Guaranteeing delete on construction failure.
• Implicit construction issues
• De-Referencing