9
\$\begingroup\$

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?
\$\endgroup\$
7
  • \$\begingroup\$ If there is something I can do to improve my question, feel free to comment any tips, don't just leave your downvote \$\endgroup\$ May 21 '17 at 1:30
  • \$\begingroup\$ I left a downvote and an edit. Wasn't that enough information? \$\endgroup\$ May 21 '17 at 1:50
  • 2
    \$\begingroup\$ @πάνταῥεῖ You just added a missing tag. Do I deserve a downvote for that? \$\endgroup\$ May 21 '17 at 2:18
  • \$\begingroup\$ Do you really need to reinvent the square wheel? Can you elaborate bout why actually? \$\endgroup\$ May 21 '17 at 3:04
  • 1
    \$\begingroup\$ Did you mean to omit the members that allow you to actually use a unique pointer? I mean, operator*() and operator->(). \$\endgroup\$ May 22 '17 at 11:06
18
\$\begingroup\$

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
            {
                moving.swap(*this);
            }
            UP& operator=(UP&& moving) noexcept
            {
                moving.swap(*this);
                return *this;
            }

            // 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
}
\$\endgroup\$
10
  • \$\begingroup\$ What would be a safer way of constructing the unique_ptrfrom a pointer of type T? \$\endgroup\$
    – dav
    Jul 20 '18 at 23:18
  • \$\begingroup\$ @DavidTran. Please look at the standard version and its interface. \$\endgroup\$ Jul 22 '18 at 6:13
  • \$\begingroup\$ @Mashpoe there is no need to check if a pointer is null before calling delete. \$\endgroup\$ Jan 14 '19 at 17:02
  • \$\begingroup\$ @U62 Compiles fine for me. In both cases they are converted to the correct type before the swap. It will only fail to compile if the class U is not derived from the class T which is exactly what it is supposed to do. \$\endgroup\$ Apr 5 '19 at 16:51
  • 1
    \$\begingroup\$ @user4893106 much of that constructor ridiculousness...: For an average class zero. In most situations the default implementation works out of the box with no need to do anything. For complex classes like this case there is usually a standard implementation already available in std::. Not sure what "simple ops only" means. \$\endgroup\$ Sep 3 '20 at 18:06
3
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ I certainly didn't plagiarize the answer, I adopted the same html design pattern to create my answer, but the content is different, obviously I forgot to put the proper reference to the two contents I mentioned the previous answer, being them #NoExcept and #It follow common C++, as my way of thinking is similar. Anyway, in these specific topics I added the reference to the original answer (@Martin York), thanks for the warning. \$\endgroup\$ Sep 22 at 19:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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