14
\$\begingroup\$

I skimmed through the documentation on cppreference.com and put down the basic functionalities expected out of a unique_ptr<T> implementation. I coded up a basic implementation std::unique_ptr<T> under 250 lines. This version is not meant to deal with arrays and custom deleters.

I would like to ask someone for a thorough code review? Does my code look like modern C++20/23? Are there any bugs in the code?

Also, how do I support a curly-braced initializer list? I am thinking to code an explicit overload for such cases.

Compiler Explorer

Implementation.

#include <iostream>
#include <memory>
#include <cassert>
#include <utility>
#include <vector>

namespace dev{
    // Implement unique_ptr here
    template<typename T>
    class unique_ptr{
        public:
        // Default c'tor 
        unique_ptr() : ptr{nullptr} {}

        // Copy c'tor should be deleted to enforce the concept that this is 
        // an owning pointer
        unique_ptr(const unique_ptr& u) = delete;

        // Copy assignment should also be deleted to enforce the ownership of the 
        // managed object
        unique_ptr& operator=(const unique_ptr& ) = delete;

        // Move constructor
        unique_ptr(unique_ptr&& other) : ptr{nullptr}
        {
            std::swap(ptr, other.ptr);
        }

        // Move assignment operator
        unique_ptr& operator=(unique_ptr&& other){
            if(ptr == other)
                return (*this);

            delete_underlying_ptr();
            ptr = std::exchange(other.ptr,nullptr);
            return (*this);
        }

        // Parameterized construtor
        unique_ptr(T* p) 
        : ptr{p} 
        {}

        // Overload deferencing operator *
        T& operator*(){
            return (*ptr);
        }

        // get the raw pointer
        T* get()
        {
            return ptr;
        }

        /* Reset the unique_ptr */
        void reset(T* other){
            if(ptr == other)
                return;

            delete_underlying_ptr();
            std::swap(ptr, other);
        }

        /* Release unique_ptr ownership */
        T* release(){
            return std::exchange(ptr,nullptr);
        }

        /* Destructor */
        ~unique_ptr(){
            delete_underlying_ptr();
        }

        /* swap - Exchange the contents of ptr1 and ptr2 member-by-member */
        friend void swap(dev::unique_ptr<T>& ptr1, dev::unique_ptr<T>& ptr2) noexcept
        {
            //Re-wire the raw pointers
            std::swap(ptr1.ptr, ptr2.ptr);
        }

        /* operator bool */
        constexpr operator bool(){
            return (ptr != nullptr);
        }

        /* Member access operator - return the underlying pointer */
        T* operator->(){
            return ptr;
        }

        /* Helper function to invoke delete on the underlying raw pointer */
        void delete_underlying_ptr(){
            if(ptr != nullptr){
                delete ptr;
                ptr = nullptr;
            }
        }
        private:
        T* ptr;
    };

    template<typename T, typename... Args>
    T* make_unique(Args&&... args){
        return new T(std::forward<Args>(args)...);
    }
}

/* Non-member functions */
//Overload operator==
template<typename T1, typename T2>
bool operator==(dev::unique_ptr<T1>& lhs, dev::unique_ptr<T2>& rhs){
    return lhs.get() == rhs.get();
}

/*template<typename T1> */
template<typename T1>
bool operator==(dev::unique_ptr<T1>& lhs, std::nullptr_t rhs){
    return lhs.get() == nullptr;
}

//Overload operator!=
template<typename T1, typename T2>
bool operator!=(dev::unique_ptr<T1>& lhs, dev::unique_ptr<T2>& rhs){
    return !(lhs == rhs);
}

template<typename T1>
bool operator!=(dev::unique_ptr<T1>& lhs, std::nullopt_t& rhs){
    return !(lhs == rhs);
}

Test Cases.

int main(){
    /* Test Cases*/

    {
        /* Non member function operator== 
           Comparison with another unique_ptr or nullptr */ 
        dev::unique_ptr<int> ptr1(new int(10));
        dev::unique_ptr<int> ptr2(new int(10));
        assert(ptr1 == ptr1);   
        assert(ptr1 != ptr2);
    }

    {
        /* Create and access */
        dev::unique_ptr<int> ptr(new int(10));
        assert(ptr != nullptr);
        assert(*ptr == 10);
        
    }

    {
        /* Reset unique_ptr 
           Replaces the managed object with a new one*/
        dev::unique_ptr<int> ptr(new int(10));
        ptr.reset(new int(20));
        assert(ptr != nullptr);
        assert(*ptr == 20);

        // Self-reset test
        ptr.reset(ptr.get());
    }

    {
        /* Release unique_ptr ownership - 
           Returns a pointer to the managed object and releases ownership */
        dev::unique_ptr<double> ptr(new double(3.14));
        double* rawPtr = ptr.release();
        assert(ptr == nullptr);
        assert(rawPtr != nullptr);
        assert(*rawPtr == 3.14);
        
    }

    {
        /* Non-member function swap
           Swap the managed objects */
        dev::unique_ptr<int> ptr1(new int(10));
        dev::unique_ptr<int> ptr2(new int (20));
        swap(ptr1, ptr2);
        assert(*ptr1 == 20);
        assert(*ptr2 == 10);
        
    }

    {
        /* Get the raw underlying pointer */
        int* x = new int(10);
        dev::unique_ptr<int> ptr(x);
        int* rawPtr = ptr.get();
        assert(*rawPtr == 10);
        assert(rawPtr == x);
        
    }

    {
        /* operator bool to test if the unique pointer owns an object */
        dev::unique_ptr<int> ptr(new int(42));
        assert(ptr);
        ptr.reset(nullptr);
        assert(!ptr);
        
    }

    {
        /* indirection operator* to dereference pointer to managed object,
           member access operator -> to call member function*/ 
        struct X{
            int n;
            int foo(){ return n; }
        };

        dev::unique_ptr<X> ptr(new X(10));
        assert((*ptr).n == 10);
        assert(ptr->foo() == 10);
        
    }

    {
        /* operator[] is used to access a managed array

        const int size = 5; 
        dev::unique_ptr<int[]> fact(new int[size]);
 
        for (int i = 0; i < size; ++i)
            fact[i] = (i == 0) ? 1 : i * fact[i - 1];

        assert(fact[0] == 1);
        assert(fact[1] == 2);
        assert(fact[3] == 6);
        assert(fact[4] == 24);
        assert(fact[5] == 120);
        */
    }

    {
        /* Constructs an object of type T and wraps it in a unique_ptr.*/
        struct Point3D{
            double x, y, z;

            Point3D() {}

            Point3D(double x_, double y_, double z_) : x(x_), y(y_), z(z_) {}

            Point3D(const Point3D& other) : x(other.x), y(other.y), z(other.z) {}

            Point3D(Point3D&& other) 
            : x(std::move(other.x))
            , y(std::move(other.y))
            , z(std::move(other.z))
            {}
        };

        // Use the default constructor. 
        dev::unique_ptr<Point3D> v1 = dev::make_unique<Point3D>();
        // Use the constructor that matches these arguments.
        dev::unique_ptr<Point3D> v2 = dev::make_unique<Point3D>(0, 1, 2);
        // Create a unique_ptr to a vector of 5 elements.
        //dev::unique_ptr<std::vector<int>> v3 = dev::make_unique<std::vector<int>>({1,2,3,4,5});
        
    }
}
\$\endgroup\$

2 Answers 2

Reset to default
15
\$\begingroup\$

First impressions are that this is pretty good code. Most of my comments will be nit-picks.

We include a lot of irrelevant headers. The unique_ptr type definition needs only <cstddef> and <utility> (also <optional> for the comparison with nullopt_t, but that's not part of std::unique_ptr's interface, and should be removed).

The test program needs <cassert>, but that shouldn't be part of the unique_ptr definition.

Comments such as these add no insight:

    // Default c'tor 

    // Move assignment operator

These should just be removed.

operator=() looks more complex than it needs to be. It's normally implemented the same as move-assignment, rather than setting the moved-from value empty. We can re-use reset() and release():

        unique_ptr& operator=(unique_ptr&& other) noexcept
        {
            reset(other.release());
            return *this;
        }

The constructor that accepts a raw pointer should be explicit. Without that, we're setting ourselves up for trouble when it is selected as a conversion and then the short-lived unique-pointer deletes the value!

The get function and the *, -> and bool operators should all be const, since they must not modify the stored pointer.

Many member functions should be noexcept - it's worth re-reading the specification of std::unique_ptr to confirm.

delete_underlying_ptr should probably be private, since it's not part of the standard interface. Its test for non-null ptr is redundant, and can be omitted:

        void delete_underlying_ptr()
        {
            delete ptr;
            ptr = nullptr;
        }

This is so simple that I question whether it needs to be a function.

reset() performs its side-effects in a different order to standard unique-pointer, which is specified to delete only _after the new pointer has been assigned.

It's helpful to make liberal use of swap() rather than manually using delete in many places in the code - when I re-implemented this, I ended up needing only one delete, in the destructor.

make_unique() needs to return a unique_ptr, not a raw pointer. As it stands, users will get a leak when they write perfectly reasonable code:

{
    auto const p = std::make_unique<>…();
    ⋮
}

The relational operators == and != should accept constant arguments. And comparison with nullptr_t should be symmetric: we need an overload with the nullptr_t as first argument.

These operators ought to be declared in the dev namespace - remember that argument-dependent lookup will find them.

The test program could be improved by using a test framework instead of assert() so that it continues to report failures rather than aborting on the first one (or perhaps ignoring all the tests, when NDEBUG is defined). Remember that assert() exists for documenting things known to be true, not for making comparisons!

The test for release() doesn't delete the released object, resulting in a Valgrind failure. That's easily fixed:

        double* rawPtr = ptr.release();
        ⋮
        delete rawPtr;

Modified code

As well as addressing the points above, the member functions and their signatures correspond exactly to those of std::unique_ptr so the type is a drop-in replacement, save for the known limitations of not supporting arrays or custom deleters.

#include <cstddef>
#include <utility>

namespace dev
{
    template<typename T>
    class unique_ptr
    {
        T* ptr;

    public:
        unique_ptr() noexcept
            : ptr{nullptr}
        {}

        explicit unique_ptr(T* p) noexcept
            : ptr{p}
        {}

        // This is a move-only type
        unique_ptr(const unique_ptr& u) = delete;
        unique_ptr(unique_ptr&& other) noexcept
            : ptr{nullptr}
        {
            swap(other);
        }

        unique_ptr& operator=(const unique_ptr&) = delete;
        unique_ptr& operator=(unique_ptr&& other) noexcept
        {
            unique_ptr tmp{std::move(other)};
            swap(tmp);
            return *this;
        }
        unique_ptr& operator=(std::nullptr_t) noexcept
        {
            unique_ptr tmp;
            swap(tmp);
            return *this;
        }

        ~unique_ptr()
        {
            delete ptr;
        }

        // ----
        T& operator*() const noexcept
        {
            return *ptr;
        }

        T* operator->() const noexcept
        {
            return ptr;
        }

        explicit operator bool() const noexcept
        {
            return ptr != nullptr;
        }

        bool operator!() const noexcept
        {
            return ptr == nullptr;
        }

        // ----
        T* get() const noexcept
        {
            return ptr;
        }

        void reset(T* other) noexcept
        {
            // N.B. Standard does not require us to detect self-reset
            unique_ptr tmp{other};
            swap(tmp);
        }

        void reset(std::nullptr_t = nullptr) noexcept
        {
            unique_ptr tmp;
            swap(tmp);
        }

        T* release() noexcept
        {
            return std::exchange(ptr, nullptr);
        }

        void swap(dev::unique_ptr<T>& other) noexcept
        {
            std::swap(ptr, other.ptr);
        }
    };


    template<typename T, typename... Args>
    unique_ptr<T> make_unique(Args&&... args)
    {
        return unique_ptr{new T(std::forward<Args>(args)...)};
    }

    template<typename T>
    void swap(unique_ptr<T>& a, unique_ptr<T>& b) noexcept
    {
        a.swap(b);
    }


    template<typename T1, typename T2>
    bool operator==(const unique_ptr<T1>& lhs, const unique_ptr<T2>& rhs)
    {
        return lhs.get() == rhs.get();
    }

    template<typename T>
    bool operator==(const unique_ptr<T>& lhs, std::nullptr_t)
    {
        return lhs.get() == nullptr;
    }

    template<typename T>
    bool operator==(std::nullptr_t, const unique_ptr<T>& rhs)
    {
        return rhs.get() == nullptr;
    }

    //Overload operator!=
    template<typename T1, typename T2>
    bool operator!=(const unique_ptr<T1>& lhs, const unique_ptr<T2>& rhs)
    {
        return !(lhs == rhs);
    }

    template<typename T>
    bool operator!=(const unique_ptr<T>& lhs, std::nullptr_t)
    {
        return lhs.get() != nullptr;
    }

    template<typename T>
    bool operator!=(std::nullptr_t, const unique_ptr<T>& rhs)
    {
        return rhs.get() != nullptr;
    }

}
\$\endgroup\$
13
  • \$\begingroup\$ Recommend using the copy-and-swap idiom for reset and move-assignment. One benefit is that you get atomicity. \$\endgroup\$ Commented Dec 25, 2024 at 0:12
  • \$\begingroup\$ @Davislor: the spec requires the moved-from value to be released, so that would still be just part of move-assignment. And a (presumably private) copy function brings risks of its own that I'd prefer to avoid! \$\endgroup\$ Commented Jan 6 at 13:48
  • \$\begingroup\$ Thanks for replying. I’m not sure why I said “copy” in my original comment. As you point out, this is a move-only type. Move-assignment can just be a swap (and the original contents would be deleted when the rvalue reference expired), and reset could construct a temporary and swap the existing object with that. If a swap is cheap and atomic, something to consider. \$\endgroup\$ Commented Jan 6 at 17:25
  • 1
    \$\begingroup\$ I've updated to use swap() more widely (still respecting the standard's requirements on deleting the moved-from value in reset()). It's certainly an improvement, so thank you! \$\endgroup\$ Commented Jan 12 at 11:44
  • 1
    \$\begingroup\$ After reading the requirements in [unique.ptr.single.modifiers], I now think I’d use your std::exchange idea from release for reset, since the Standard specifies that get_deleter must be called, possibly with side-effects. That is, get_deleter()(std::exchange(ptr, other));. This implementation doesn’t have deleters, so the simplified implementation works, but it could easily be added as a stub that return std::default_delete<T>{};. \$\endgroup\$ Commented Jan 12 at 19:31
1
\$\begingroup\$

Updated: this convention is outdated and OP's original code did it better.

The convention for swap, instead of declaring it friend, is to provide a public member function swap as well as specializing std::swap for your type.

namespace std {
    template<> void swap<Widget>(Widget& a, Widget& b) 
    {
        a.swap(b); 
    }
}

Your class is templated and you can't partially specialize in std yourself. So you would have to provide a non-member swap function outside your class in your namespace.

template<typename T> void swap(unique_ptr<T>& a, unique_ptr<T>& b) 
{
    a.swap(b); 
}

Reference: Effective C++ 55 Specific Ways to Improve Your Programs and Designs, Item 25

\$\endgroup\$
5
  • 1
    \$\begingroup\$ You shouldn’t specialize std::swap() unless it is absolutely necessary, and it isn’t in this case. Making a hidden friend swap() function—like was done in the original code—is the right thing to do. \$\endgroup\$ Commented Dec 26, 2024 at 5:15
  • \$\begingroup\$ @indi I got that specific advice from "Effective C++ 55 Specific Ways to Improve Your Programs and Designs," item 25. Do you have any reasons against that advice? \$\endgroup\$ Commented Dec 26, 2024 at 17:33
  • 2
    \$\begingroup\$ All of the Effective C++ books are extremely dated. Scott Meyers actually retired from C++ ~10 years ago. and the hidden friends technique wasn’t even invented until after that. Nowadays, the correct way to implement swap for a type is not to crack open std and specialize std::swap(), but rather to implement a hidden friend swap() function in the class (just like was done in the original code). This will work with the old-school “swap two-step” or the modern std::ranges::swap() without unexpected conversions. \$\endgroup\$ Commented Dec 26, 2024 at 20:13
  • 1
    \$\begingroup\$ Just noticed the extension to your answer, and saw some misconceptions. You can partially specialize templates in std, subject to all the normal rules for specializing in std. This is why struct<template T> std::hash<foo<T>> { /* ... */ }; is fine. You can’t add new overloads to std. There is no such thing as partially specializing function templates; template<typename T> auto std::swap<foo<T>>(foo<T>&, foo<T>&) {} is not valid C++. template<typename T> auto std::swap(foo<T>&, foo<T>&) {} is an overload, and thus, illegal. (Godbolt) \$\endgroup\$ Commented Dec 27, 2024 at 13:59
  • \$\begingroup\$ I was saying that since std::swap was a templated function, OP couldn't partially specialize it. \$\endgroup\$ Commented Dec 27, 2024 at 17:15

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.