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
};
200_success
145k22 gold badges190 silver badges478 bronze badges
asked May 6, 2019 at 2:56
\$\endgroup\$
1
  • \$\begingroup\$ Please read my series on smart pointers for some help: lokiastari.com/series \$\endgroup\$ Commented May 6, 2019 at 19:18

1 Answer 1

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.

answered May 6, 2019 at 7:15
\$\endgroup\$
0

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.