5
\$\begingroup\$

Took a shot at implementing std::unique_ptr.

Code:

#include <memory>
#include <stdexcept>

template<typename T>
struct DefaultDeleter {
    void operator()(T* ptr) const  {
        delete ptr;
    }
};

template <typename T>
struct DefaultDeleter<T[]> {
    void operator()(T* ptr) const {
        delete[] ptr;
    }
};

template<typename T, typename Deleter = DefaultDeleter<T>>
class UniquePtr {
public:
    using pointer = T*;
    using element_type = T;
    using deleter_type = Deleter;

    constexpr UniquePtr() noexcept : m_ptr{nullptr} {}
    explicit UniquePtr (pointer p) noexcept : m_ptr{p} {}
    explicit UniquePtr (pointer p, const deleter_type& deleter) :
        m_ptr{p}, 
        m_deleter{deleter} 
    {}

    UniquePtr(const UniquePtr& other) = delete;
    UniquePtr& operator=(const UniquePtr& other) = delete; 

    UniquePtr(UniquePtr&& other) noexcept :
        m_ptr{other.release()}, 
        m_deleter{std::move(other.m_deleter)} 
    {}

    // Added this to allow for runtime polymorphism, but I didn't 
    // see such a constructor on cppreference. Am I just missing something
    // or should this be handled another way? 
    template<typename U, 
        std::enable_if_t<std::is_convertible_v<U*, T*>, bool> = false>
    UniquePtr(UniquePtr<U>&& other) noexcept :
        m_ptr{other.release()} 
    {}

    UniquePtr& operator=(UniquePtr&& other) noexcept {
        if (this != &other) {
            m_deleter(m_ptr);
            m_ptr = other.m_ptr;
            other.m_ptr = nullptr;
            m_deleter = std::move(other.m_deleter);
        }

        return *this;
    }

    pointer release() noexcept {
        pointer result = m_ptr;
        m_ptr = nullptr;
        return result;
    }

    void reset(pointer ptr = pointer()) noexcept {
        pointer old_ptr = m_ptr;
        m_ptr = ptr;
        if (old_ptr) {
            m_deleter(old_ptr);
        }
    }

    void swap(UniquePtr& other) noexcept {
        std::swap(m_ptr, other.ptr_);
        std::swap(m_deleter, other.m_deleter);
    }

    pointer get() const noexcept {
        return m_ptr;
    }

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

    std::add_lvalue_reference_t<element_type> operator* () const 
        noexcept(noexcept(*std::declval<pointer>())) {
        return *m_ptr;
    }

    pointer operator->() const noexcept {
        return m_ptr;
    }

    ~UniquePtr() {
        m_deleter(m_ptr);
    }
private:
    pointer m_ptr;
    deleter_type m_deleter;
};


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

Some tests, mostly attempting to show the example functionality on cppreference.com works with my implementation. Plus a couple extra tests to fill in gaps/test things a bit more precisely.

#include <cassert>
#include <fstream>
#include <iostream>
#include "UniquePtr.h"

/* Test setup code (adapted from cppreference.com) */

// helper class for runtime polymorphism demo below
struct B
{
    virtual ~B() = default; 
    virtual int val() const {return 0;}
};
 
struct D : B
{
    D() { }
    ~D() {}
 
    int val() const override {
        // dummy value to demonstrate dereferencing
        return 2;
    }
};
 
// a function consuming a unique_ptr can take it by value or by rvalue reference
UniquePtr<D> pass_through(UniquePtr<D> p)
{
    p->val();
    return p;
}
 
// helper function for the custom deleter demo below
void close_file(std::FILE* fp)
{
    std::fclose(fp);
}

struct Node
{
    int data;
    UniquePtr<Node> next;
};
// unique_ptr-based linked list demo
struct List
{
    UniquePtr<Node> head;
 
    ~List()
    {
        // destroy list nodes sequentially in a loop, the default destructor
        // would have invoked its `next`'s destructor recursively, which would
        // cause stack overflow for sufficiently large lists.
        while (head)
        {
            
            // std::cout << "before first move \n";
            auto next = std::move(head->next);
            // std::cout << "before second move \n";
            head = std::move(next);
        }
    }
 
    void push(int data)
    {
        head = UniquePtr<Node>(new Node{data, std::move(head)});
    }
};

/* Beginning of tests */

void testOwnershipTransfer() {
    UniquePtr<D> p = makeUnique<D>();
    UniquePtr<D> q = pass_through(std::move(p));
    assert(!p);
}

void testNullUniquePtr() {
    UniquePtr<D> p;
} // this function should exit successfully 

void testDererference() {
    UniquePtr<D> p = makeUnique<D>();
    D obj = *p;
    assert(obj.val() == 2);
}

void testBool() {
    UniquePtr<D> p;
    assert(!p);

    p = makeUnique<D>();
    assert(p);

    p.reset();
    assert(!p);
}

void testRuntimePolymorphism() {
    UniquePtr<B> p = makeUnique<D>();
    assert(p -> val() == 2);  
}

void testFileDeletion() {
    std::ofstream("test.txt") << 'x'; // prepare the file to read
    {
        using unique_file_t = UniquePtr<std::FILE, decltype(&close_file)>;
        unique_file_t fp(std::fopen("test.txt", "r"), &close_file);
        if (fp) {
            assert(char(std::fgetc(fp.get())) == 'x');
        }
    } // `close_file()` called here (if `fp` is not null)
}

void testExceptionSafety() {
    try
    {
        UniquePtr<D, void(*)(D*)> p(new D, [](D* ptr)
        {
            delete ptr;
        });
 
        throw std::runtime_error(""); // `p` would leak here if it were a plain pointer
    }
    catch (const std::exception&) { std::cout << "Caught exception\n"; }
}

void testLinkedList() {
    List wall;
    for (int i = 0; i != 1'000'000; ++i)
        wall.push(i);
} // function should exit successfully 


int main()
{
    testOwnershipTransfer();
    testNullUniquePtr();
    testDererference();
    testBool();
    testRuntimePolymorphism();
    testFileDeletion();
    testExceptionSafety();
    testLinkedList();
}

This omits the array specialization, because when taking a peek at the GCC implementation it seems to be mostly a duplication of the class with some variations here and there to support arrays. Is this how it needs to be done, or is it possible to reuse the existing code somehow and write a small, concise class for the array specialization?

Thanks in advance to anyone who can take a look!

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

The converting constructor you added is part of the interface of std::unique_ptr. It is number (6) of the list on cppreference. However you should change yours to also accept UniquePtr's with a different deleter. But you have to be somewhat pedantic to exhibit the required behaviour if Deleter is a reference type.

// To access private members of UniquePtr's with different template parameters
template <typename, typename> friend class UniquePtr;

// Converting constructors
// Version 1 for non-reference deleter types
template<typename U, typename E,
    std::enable_if_t<std::is_convertible_v<U*, T*>, bool> = false,
    std::enable_if_t<!std::is_reference_v<Deleter> && std::is_convertible_v<E, Deleter>,
                     bool> = false>
UniquePtr(UniquePtr<U, E>&& other) noexcept :
    m_ptr{other.release()}, m_deleter{std::move(other.m_deleter)} {}

// Converting constructors
// Version 2 for reference deleter types
template<typename U, typename E,
    std::enable_if_t<std::is_convertible_v<U*, T*>, bool> = false,
    std::enable_if_t<std::is_reference_v<Deleter> && std::is_same_v<E, Deleter>,
                     bool> = false>
UniquePtr(UniquePtr<U, E>&& other) noexcept :
    m_ptr{other.release()}, m_deleter{other.m_deleter} {}

However for the new converting constructors to work, we need to specify a conversion for the different deleter types involved:

DefaultDeleter() = default;
    
template <typename U,
          std::enable_if_t<std::is_convertible_v<U*, T*>, bool> = true>
DefaultDeleter(DefaultDeleter<U> const&) {}

Probably most of the time Deleter will be DefaultDeleter<T>. DefaultDeleter<T> is an empty type. But since it's a member of UniquePtr, C++ requires it to have a unique memory address. Because of this your UniquePtr will have size 16 instead of size 8 (on 64-bit systems), with the second 8 bytes being wasted space. To solve this, you could add the [[no_unique_address]] attribute. Unfortunately, microsofts C++ compiler does not honour this attribute. Another solution is to exploit empty base class optimization, which all major C++ compilers support. Make a private base class of UniquePtr, that has the deleter as a member or as a base class, depending on whether Deleter is a class type or not.

template <typename Deleter, bool = std::is_class_v<Deleter> && !std::is_final_v<Deleter>>
struct DeleterHolder: private Deleter {
    // Use perfect forwarding for construction with class types
    template <typename E>
    DeleterHolder(E&& e): Deleter(std::forward<E>(e)) {}

    Deleter& get_deleter() noexcept {
        return static_cast<Deleter&>(*this);
    }

    const Deleter& get_deleter() const noexcept {
        return static_cast<Deleter const&>(*this);
    }
};

template <typename Deleter>
struct DeleterHolder<Deleter, false> {
    // Simply pass pointers and references 'by value'
    DeleterHolder(Deleter d): m_deleter(d) {}

    Deleter& get_deleter() noexcept {
        return this->m_deleter;
    }

    const Deleter& get_deleter() const noexcept {
        return this->m_deleter;
    }

private:
    Deleter m_deleter;
};

template<typename T, typename Deleter = DefaultDeleter<T>>
class UniquePtr: DeleterHolder<Deleter> { 
public:
    using DeleterHolder<Deleter>::get_deleter;

    // Rest of the implementation...
};

Now you have to go through all the places where you use the deleter and update the code there to use get_deleter and the constructor of DeleterHolder.

We do not unconditionally inherit from Deleter, because it might be a function type, which you cannot inherit from.

Thanks to @Deduplicator for pointing out an issue in the previous version of the code here :-)


Otherwise the implementation looks fine from what I can see :-)

It is great that you added tests. But you should add more tests that directly verify that the contracts of the smart pointer are fulfilled. For example, check that objects always get deleted when the pointer goes out of scope, check that no double deletion happens etc. Also you might want to consider to use a test framework such as Catch2 or google test.

\$\endgroup\$
9
  • \$\begingroup\$ DeleterHolder is not empty, as it has a (though empty) member which is not a base. Still, you are on the right track for using EBO where [[no_unique_address]] is not honored. \$\endgroup\$ Jul 25, 2023 at 14:04
  • \$\begingroup\$ @Deduplicator Thanks for pointing this out, I hoped there was a way to do it without two implementations for class types and non-class types. \$\endgroup\$
    – chrysante
    Jul 25, 2023 at 14:16
  • \$\begingroup\$ One implementation for empty non-final class types, one for the rest... \$\endgroup\$ Jul 25, 2023 at 14:30
  • 1
    \$\begingroup\$ @jdav22 1. I assume it might be to prevent accidental copies of the deleter, but I don't really know. I was just following the standard here. 2. There is no point in calling std::move, since argument and parameter are both the same l-value-reference type. std::move would cast to rvalue-reference and that be implicitly be converted to lvalue again. \$\endgroup\$
    – chrysante
    Jul 26, 2023 at 7:40
  • 1
    \$\begingroup\$ 3. By requiring our deleters to be convertible, we make them sign a contract. By making Deleter<U> be convertible to Deleter<T>, the author of Deleter promises that Deleter<T> can delete pointer of type U* after being converted to T*. And the fact that DefaultDeleter<T> is empty does not mean that all possible deleters are empty. 4. It doesn't matter here if we inherit privately or publicly. I just did it because data hiding is generally a good habit, but in this case it doesn't really change anything. \$\endgroup\$
    – chrysante
    Jul 26, 2023 at 7:53
0
\$\begingroup\$

Use the Swap Idiom to Move Instances

Among other benefits, swaps can be thread-safe atomic operations that never throw exceptions.

About That Converting Constructor

(Thanks to @chrysante for pointing out a serious oversight in my original answer.)

One important piece of functionality here is the ability to do things like:

 std::unique_ptr<Base> foo = std::make_unique<Derived>(bar, baz);

You implement this with the converting constructor

    // Added this to allow for runtime polymorphism, but I didn't 
    // see such a constructor on cppreference. Am I just missing something
    // or should this be handled another way? 
    template<typename U, 
        std::enable_if_t<std::is_convertible_v<U*, T*>, bool> = false>
    UniquePtr(UniquePtr<U>&& other) noexcept :
        m_ptr{other.release()} 
    {}

First, in a reinventing-the-wheel project, you get to use requires from C++20, which is much nicer than std::enable_if. So I recommend that.

Second, this implementation will break if the pointer conversion changes the bits of the pointer, even if it is from a derived class to a base class with a virtual destructor. This is because not all pointer conversions are pointer-interconvertible. That is, some pointer conversions (such as multiple or virtual inheritance) produce a pointer with different bits. Calling the original deleter, which expects a T* and not a U*, with the converted U*, would then fail. The current approach is only safe in two cases:

  • Where std::is_pointer_interconvertible_base_class<T,U> is true and the destructor of T is either virtual or trivial, or
  • Where std::is_layout_compatible<T,U>, std::is_trivially_destructible<T> and std::is_trivially_destructible<U> are all true. (The standard does not say std::unique_ptr should support this case.)

At present, however, the implementation supports this for all possible conversions, including arbitrary one-way custom conversion operators. The only way to make this work robustly would be to store the closure of the deleter and the original pointer inside each instance.

If you aren’t going to go through all that, you could declare non-pointer-interconvertible aliasing unsupported, add a check for that to your restrict clauses, and keep the simple, high-performance implementation you have now. In that case, you want to declare your deleter with the [[no_unique_address]] attribute, so that the vast majority of deleters, which are unit types whose instance contain nothing, don’t need to be created and stored as objects.

Otherwise, you could put a/*** BIG IMPORTANT WARNING ***/ in your header file that people are only supposed to pass in a deleter that still works, somehow, with every possible conversion to a base pointer. And then say, the bugs this causes are on them for not reading the documentation.

Finally, since you ask, the converting constructor appears to be number 6 on Cppreference, as of July 2023.

\$\endgroup\$
9
  • \$\begingroup\$ I'm confused. OP already has a converting constructor as far as I can see. And why would you go through any of the hassle with std::is_pointer_interconvertible_base_of? The standard does not require the deleter of the old unique_ptr to be invoked with the old value of the pointer when the new unique_ptr is destroyed. I.e. if Base* q = p has a different value than Derived* p, delete q still works fine and deletes the object as long as Base has a virtual destructor. Also wrapping every call to delete in a std::function can be a nightmare for performance. \$\endgroup\$
    – chrysante
    Jul 24, 2023 at 21:51
  • \$\begingroup\$ @chrysante Thanks for pointing out the converting constructor. It’s defined with std::enable_if, which I wasn’t looking for, and I missed it. (I should correct this and suggest the use of requires.) \$\endgroup\$
    – Davislor
    Jul 24, 2023 at 22:30
  • \$\begingroup\$ @chrysante It is, unfortunately, not the case that calling the provided deleter with static_cast<Base*>(p) will necessarily work. Examples where it wouldn’t include some implementations of multiple and virtual inheritance. If the base class is not pointer-interconvertible, (Base*)p might have different bits than p, which the arbitrary deleter might not recognize. \$\endgroup\$
    – Davislor
    Jul 24, 2023 at 22:33
  • \$\begingroup\$ @chrysante If you don’t want to take the performance penalty of storing a std::function in each instance of unique_ptr, OP needs to restrict the conversions to types that will actually work with the implementation. That is: pointer-interconvertible base classes, and layout-compatible types. \$\endgroup\$
    – Davislor
    Jul 24, 2023 at 22:36
  • \$\begingroup\$ I think std::unique_ptr solves this problem by requiring the deleters to be convertible. std::unique_ptr<T, D> p = std::unique_ptr<U, E>(/*...*/); is only valid if U* is convertible to T* and if E is convertible to D. And by allowing conversions between your different deleter types, you implicitly say: D can delete all pointers that E can delete (after being converted to the type of pointer D can delete). So worrying about the deleters being compatible is not the responsibility of unique_ptr but that of the deleters. \$\endgroup\$
    – chrysante
    Jul 24, 2023 at 22:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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