4 added 612 characters in body
source | link
  • 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.

  • 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.

  • 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.

  • 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.

3 deleted 28 characters in body
source | link
  • 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 enabledefine them (= default is sufficient). 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.

  • 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.

  • 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 enable them (= default is sufficient). 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.

  • 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.

  • 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.

  • 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.

2 added 91 characters in body
source | link
  • 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 enable them (= default is sufficient). 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.

  • TheExcept 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_tr<SomeType>unique_ptr<SomeType> empty{nullptr};
    

    which simply invokes the first constructor taking a T* argument. So 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.

  • 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 enable them (= default is sufficient). 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.

  • The constructor taking a std::nullptr_t is superfluous. You can construct an empty unique_ptr by

    unique_tr<SomeType> empty{nullptr};
    

    which simply invokes the first constructor taking a T* argument. So 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.

  • 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 enable them (= default is sufficient). 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.

  • 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.

1
source | link