5
\$\begingroup\$

This is my simple unique_ptr implementation. Anything that could be improved upon or should be added?

#include <algorithm>

template<typename T>
class unique_ptr
{
private:
    T * ptr_resource = nullptr;
public:
    // Safely constructs resource. Operator new is called by the user. Once constructed the unique_ptr will own the resource.
    // std::move is used because it is used to indicate that an object may be moved from other resource.
    explicit unique_ptr(T* raw_resource) noexcept : ptr_resource(std::move(raw_resource)) {}
    unique_ptr(std::nullptr_t) : ptr_resource(nullptr) {}

    // destroys the resource when object goes out of scope
    ~unique_ptr() noexcept
    {
        delete ptr_resource;
    }
    // Disables the copy/ctor and copy assignment operator. We cannot have two copies exist or it'll bypass the RAII concept.
    unique_ptr(const unique_ptr<T>&) noexcept = delete;
    unique_ptr& operator = (const unique_ptr&) noexcept = delete;
public:
    // releases the ownership of the resource. The user is now responsible for memory clean-up.
    T* release() noexcept
    {
        T* resource_ptr = this->ptr_resource;
        this->ptr_resource = nullptr;

        return resource_ptr;
    }
    // returns a pointer to the resource
    T* get() const noexcept
    {
        return ptr_resource;
    }
    // swaps the resources
    void swap(unique_ptr<T>& resource_ptr) noexcept
    {
        std::swap(ptr_resource, resource_ptr.ptr_resource);
    }
    // replaces the resource. the old one is destroyed and a new one will take it's place.
    void reset(T* resource_ptr) noexcept(false)
    {
        // ensure a invalid resource is not passed or program will be terminated
        if (resource_ptr == nullptr)
            throw std::invalid_argument("An invalid pointer was passed, resources will not be swapped");

        delete ptr_resource;

        ptr_resource = nullptr;

        std::swap(ptr_resource, resource_ptr);
    }
public:
    // operators
    T* operator->() const noexcept
    {
        return this->ptr_resource;
    }
    T& operator*() const noexcept
    {
        return *this->ptr_resource;
    }
    // May be used to check for nullptr
};
\$\endgroup\$
7
\$\begingroup\$
  • Let me first assume that your unique_ptr is supposed to be movable. Then, any basic test case whould have unrevealed this:

    unique_ptr<int> ptr1(new int());
    unique_ptr<int> ptr2 = std::move(ptr1); // Fails to compile
    

    Recall that = delete-ing special member functions means user-declaring them. And user-declared copy and copy assignment constructors prevent compiler-generated move (assignment) constructors! You have to manually define them. This case is by the way covered by the rule of five/C.21 Core Guidelines, and also have a look at the table posted in this SO answer for an overview of compiler-generated/-deleted/not-declared special member functions.

  • Is the non-availability of an implicit conversion to bool intended? Checking if a (smart) pointer is in an empty/null state is so common in ordinary control flow statements that clients will expect this to compile:

    unique_ptr<SomeType> ptr = ...;
    
    if (ptr) ... // currently fails to compile
    

    But not that this might be debatable. Implicit conversions can cause a lot of pain, so if you intend to not allow them for the sake of a more explicit

    if (ptr == nullptr) ... 
    

    that's a design decision. But one that should be documented in a comment at the top of the class.

  • Except the non-explicit-ness of the second constructor (thanks to @Deduplicator for pointing that out) taking a std::nullptr_t, it is superfluous. You can construct an empty unique_ptr by

    unique_ptr<SomeType> empty{nullptr};
    

    which simply invokes the first constructor taking a T* argument. I would remove the second constructor.

  • ... and add a default constructor that initializes ptr_resource to nullptr, as

    unique_tr<SomeType> empty;
    

    might be a way of constructing an empty smart pointer that users would expect to compile.

  • Move-constructing the ptr_resource in the constructor initializer by ptr_resource(std::move(raw_resource)) doesn't make much sense. Just copy the pointer instead. The comment // std::move is used because it is used to indicate that an object may be moved from other resource. is rather confusing, because T* raw_resource is already a pointer, and hence a handle to a resource, not the resource itself.

  • The release member function can be implemented more conveniently as

    T* release() noexcept
    {
        return std::exchange(ptr_resource, nullptr);
    }
    
  • I wouln't let the reset member function throw when the input is a nullptr. Why shouldn't it be allowed to reset a unique_ptr with a nullptr, turning it back into an empty state?

  • The only facilities you use from the standard library are std::move and std::swap. Those are in <utility>, so you don't need to include <algorithm>, which is probably much heavier in terms of compile times.

  • I would omit the this-> prefix, it's unnecessarily verbose. But that might be a matter of taste.

  • Have you considered custom deleters? This makes the class template more reusable in scenarios other than pointers to heap resources, e.g. closing a file upon destruction etc.

\$\endgroup\$

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.