My implementation for std::unique_ptr

Question

I just finished learning about move semantics and realized that a nice practical example for this concept is unique_ptr (it cannot be copied, only moved).

For learning purposes, and as a personal experiment, I proceed to try to create my implementation for a smart unique pointer:

template<typename T>
class unique_ptr {
private:
    T* _ptr;
public:
    unique_ptr(T& t) {
       _ptr = &t;
    }
    unique_ptr(unique_ptr<T>&& uptr) {
       _ptr = std::move(uptr._ptr);
       uptr._ptr = nullptr;
    }
    ~unique_ptr() {
       delete _ptr;
    }
    unique_ptr<T>& operator=(unique_ptr<T>&& uptr) {
       if (this == uptr) return *this;
       _ptr = std::move(uptr._ptr);
       uptr._ptr = nullptr;
       return *this;
    }

    unique_ptr(const unique_ptr<T>& uptr) = delete;
    unique_ptr<T>& operator=(const unique_ptr<T>& uptr) = delete;
};

For a small set of test cases, this is working like the real unique_ptr.

However, it just seems too simple enough.

I have two questions regarding this code:

1. Is it well-formed? i.e. does it follow common C++ standard and patterns (for example, should private members be declared before public ones?
2. Am I missing something regarding functionality? Is there maybe a bug in my code that I'm not seeing?

Answer 1

Overall

Ohhhhh.

Is it well-formed?

It compiles, so yes.

i.e. does it follow common C++ standard and patterns (for example, should private members be declared before public ones?

Personally I think so.

* Private Variables
* Public
    * Constuctor / Destructor
    * Copy Semantics
    * Move Semantics
    * Swap
    * Other Public Interface
    * Friends
* Private
    * Methods as appropriate

When reading the code I want to know the members so I can verify that the constructors initialize them all, as a result I usually put them first. But other people prefer to put all private stuff at the bottom.

Am I missing something regarding functionality?

Yes. Quite a lot.

Is there maybe a bug in my code that I'm not seeing?

Yes. It potentially leaks on assignment.

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 unwritten 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 overwrite _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 pessimization

    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 pessimization 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_ptr 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
• Dereferencing

When you have read all three articles then the bare bones unique_ptr looks like this:

namespace ThorsAnvil
{
    template<typename T>
    class UP
    {
        T*   data;
        public:
            UP()
                : data(nullptr)
            {}
            // Explicit constructor
            explicit UP(T* data)
                : data(data)
            {}
            ~UP()
            {
                delete data;
            }

            // Constructor/Assignment that binds to nullptr
            // This makes usage with nullptr cleaner
            UP(std::nullptr_t)
                : data(nullptr)
            {}
            UP& operator=(std::nullptr_t)
            {
                reset();
                return *this;
            }

            // Constructor/Assignment that allows move semantics
            UP(UP&& moving) noexcept
                : data(nullptr)
            {
                moving.swap(*this);
                // In the comments it was pointed out that this
                // does not match the implementation of std::unique_ptr
                // I am going to leave mine the same. But
                // the the standard provides some extra guarantees
                // and probably a more intuitive usage.
            }
            UP& operator=(UP&& moving) noexcept
            {
                moving.swap(*this);
                return *this;
                // See move constructor.
            }

            // Constructor/Assignment for use with types derived from T
            template<typename U>
            UP(UP<U>&& moving)
            {
                UP<T>   tmp(moving.release());
                tmp.swap(*this);
            }
            template<typename U>
            UP& operator=(UP<U>&& moving)
            {
                UP<T>    tmp(moving.release());
                tmp.swap(*this);
                return *this;
            }

            // Remove compiler generated copy semantics.
            UP(UP const&)            = delete;
            UP& operator=(UP const&) = delete;

            // Const correct access owned object
            T* operator->() const {return data;}
            T& operator*()  const {return *data;}

            // Access to smart pointer state
            T* get()                 const {return data;}
            explicit operator bool() const {return data;}

            // Modify object state
            T* release() noexcept
            {
                T* result = nullptr;
                std::swap(result, data);
                return result;
            }
            void swap(UP& src) noexcept
            {
                std::swap(data, src.data);
            }
            void reset()
            {
                T* tmp = release();
                delete tmp;
            }
    };
    template<typename T>
    void swap(UP<T>& lhs, UP<T>& rhs)
    {
        lhs.swap(rhs);
    }
}

Test to make sure it compiles:

struct X {}; 
struct Y: public X {}; 

int main()
{
    ThorsAnvil::UP<X>   x(new X); 
    ThorsAnvil::UP<Y>   y(new Y); 

    x   = std::move(y);                  // This should be valid
    ThorsAnvil::UP<X>   z(std::move(y)); // This should be valid

    // In both these cases x represents an X* which is a base pointer
    // that should be able to point at objects of type Y* as these
    // are derived from X
}

Answer 2

Review

The code is well formatted and also reasonably well implemented, lacking only a few security overhauls such as memory leaks and optimizations also completing the pending implementations of existing concepts in unique_ptr

Is it well-formed?

Yes, have good formatting

i.e. does it follow common C++ standard and patterns (for example, should private members be declared before public ones?

I have the same thought present in Martin York' answer:

Personally I think so.

* Private Variables
* Public
    * Constuctor / Destructor
    * Copy Semantics
    * Move Semantics
    * Swap
    * Other Public Interface
    * Friends
* Private
    * Methods as appropriate

When reading the code I want to know the members so I can verify that the constructors initialize them all, as a result I usually put them first. But other people prefer to put all private stuff at the bottom.

Am I missing something regarding functionality?

Yes, you forgot to add T, and T[], and the following features:

type* release();

void reset(type* item);

void swap(unique_ptr &other);

type* get();

operator*;

operator->;

operator[];

Is there maybe a bug in my code that I'm not seeing?

Yes, you must force the receiving of types strictly to be pointers

Code Review

Copy assignment operator

unique_ptr<T>& operator=(const unique_ptr<T>& uptr) = delete;

This is redundant, this part is not necessary as this operator will never be called taking into account that the motion constructor exists and the copy constructor is disabled (= delete).

Template only typename T

template<typename T>
class unique_ptr {
   ...
}

You must accept both types T, and T[], ie: array or not.

Constructing from object

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

That's exceedingly dangerous:

int x;
unique_ptr<int>  data(x);
//The unique ptr calls delete on an automatic variable.

NoExcept

I have the same thought present in @Martin York' answer:

unique_ptr(unique_ptr<T>&& uptr) noexcept
                                   //^^^^^^^^
        
unique_ptr<T>& operator=(unique_ptr<T>&& uptr) 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.

Verify in move assignment operator

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

Before _ptr =, you need to check if this->ptr is initialized, if yes delete ptr before assign.

if (this == uptr) return *this;

This above code is incorrect, this is pointer and uptr is reference, you need to add &other to verify successfully

Note: std::move in uptr._ptr is irrelevant.

Example code

I'll leave the complete code below based on your initial reasoning, I've added the outstanding parts and fixed the compatibility issue

#include <iostream>

using namespace std;

template<typename T>
class base_ptr {
private:
    using type = remove_extent_t<T>;

protected:
    type* ptr = nullptr;

public:
    base_ptr(){};
    
    base_ptr(base_ptr<T>&& other) noexcept
    : ptr(other.ptr) {
       other.ptr = nullptr;
    }

    type* release(){
       type* tmp = ptr;
       ptr = nullptr;
       return tmp;
    }

    void reset(){
       if(ptr != nullptr){
           if(is_array_v<T>)
               delete[] ptr;
            else
               delete ptr;
       }
       
       ptr = nullptr;
    }

    void reset(type* item) noexcept {
       reset();

       ptr = item;
    }

    void swap(base_ptr<T> &other) noexcept {
       type* tmp = ptr;

       ptr = other.ptr;
      
       other.ptr = tmp;
    }

    operator bool(){
       return ptr;
    }

    type* get(){
        return ptr;
    }

    base_ptr<T>& operator=(base_ptr<T>&& other) noexcept {
       if (this == &other) return *this;
      
       reset();
       ptr = other.ptr;
       other.ptr = nullptr;
      
       return *this;
    }
};

template <typename T>
class unique_ptr : public base_ptr<T> {
public:
    unique_ptr(){}

    unique_ptr(unique_ptr<T>&& other) 
    : base_ptr<T>(move(other)) { }

    explicit unique_ptr(T *ptr){
       base_ptr<T>::ptr = ptr;
    }

    unique_ptr<T>& operator=(unique_ptr<T>&& other){
       base_ptr<T>::operator=( move(other) );

       return *this;
    }

    T* operator->() const {
       return base_ptr<T>::ptr;
    }

    T& operator*() const {
       return *base_ptr<T>::ptr;
    }

    ~unique_ptr(){
       base_ptr<T>::reset();
    }
};

template <typename T>
class unique_ptr<T[]> : public base_ptr<T[]> {
public:
    unique_ptr(){}

    unique_ptr(unique_ptr<T[]>&& other) 
    : base_ptr<T[]>(move(other)) { }

    explicit unique_ptr(T *ptr){
       base_ptr<T[]>::ptr = ptr;
    }

    unique_ptr<T[]>& operator=(unique_ptr<T[]>&& other){
       base_ptr<T[]>::operator=( move(other) );

       return *this;
    }

    T& operator[](int pos) const {
       return base_ptr<T[]>::ptr[pos];
    }

    ~unique_ptr(){
       base_ptr<T[]>::reset();
    }
};

template <class T, class ...Args>
enable_if_t<!is_array_v<T>, unique_ptr<T>>
make_unique(Args&& ...args)
{
   return unique_ptr<T>(new T(forward<Args>(args)...));
};

template <class T>
enable_if_t<is_array_v<T>, unique_ptr<T>>
make_unique(int size)
{
   using type = remove_extent_t<T>;
   return unique_ptr<T>(new type[size]);
};

int main() {
    unique_ptr<int> a (new int(1));
  
    unique_ptr<int> b;

    b.swap(a);

    b = make_unique<int>(2);

    b.reset();
  
    cout << "End" << endl;
  
    return 0;
}