4
\$\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\$
  • \$\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\$ – Matias Cicero 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
  • \$\begingroup\$ @πάνταῥεῖ You just added a missing tag. Do I deserve a downvote for that? \$\endgroup\$ – Matias Cicero 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\$ – Toby Speight May 22 '17 at 11:06
9
\$\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\$
  • \$\begingroup\$ What would be a safer way of constructing the unique_ptrfrom a pointer of type T? \$\endgroup\$ – David Tran Jul 20 '18 at 23:18
  • \$\begingroup\$ @DavidTran. Please look at the standard version and its interface. \$\endgroup\$ – Martin York Jul 22 '18 at 6:13
  • \$\begingroup\$ @Mashpoe there is no need to check if a pointer is null before calling delete. \$\endgroup\$ – Martin York Jan 14 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\$ – Martin York Apr 5 at 16:51

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.