6
\$\begingroup\$

I am writing my simple shared pointer. I am asking to review existing functions (understand that my implementations is not full, e.g now operator*)

Review please correctness

  1. Copy/move constructors and operator=
  2. Destructors

My code

template<typename T>
class my_shared_ptr {
public:
    my_shared_ptr() {}

    my_shared_ptr(T* ptr) {
        ptr_ = ptr;
        counter_ = new size_t(1);
    }

    my_shared_ptr(const my_shared_ptr& other) {
        destroy_current();
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        if (other.ptr_ != nullptr) {
            *counter_ += 1;
        }
    }

    my_shared_ptr(my_shared_ptr&& other) {
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        
        other.ptr_ = nullptr;
        other.counter_ = nullptr;
    }

    my_shared_ptr& operator=(const my_shared_ptr& other) {
        destroy_current();
        
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        if (other.ptr_ != nullptr) {
            *counter_ += 1;
        }
    }

    my_shared_ptr&& operator=(my_shared_ptr&& other) {
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        
        other.ptr_ = nullptr;
        other.counter_ = nullptr;
    }

    ~my_shared_ptr() {
        destroy_current();
    }
private:
    void destroy_current() {
        if (counter_ and *counter_ > 1) {
            *counter_ -= 1;
        } else {
            delete ptr_;
            delete counter_;
        }
    }
private:
    T* ptr_ = nullptr;
    size_t* counter_ = nullptr;
};


template<typename T, typename... Args>
my_shared_ptr<T> make_my_shared(Args&&... args) {
    auto ptr = new T(std::forward<Args>(args)...);
    return my_shared_ptr<T>(ptr);
}


struct A {
  int val_ = 0;
  A(int val) : val_(val) { std::cout << "A(" << val_ << ")" << std::endl ; }
  ~A() { std::cout << "~A(" << val_ << ")" << std::endl ; }

};


int main() {
  std::cout << "-----------\n";
  {
    my_shared_ptr<A> a1;
    //my_shared_ptr<A> a2 = make_my_shared<A>(2); 
    my_shared_ptr<A> a2(new A(2));
    {
      my_shared_ptr<A> a3(new A(3));
      std::cout << "log 1" << std::endl;
      a1 = a3;
      std::cout << "log 2" << std::endl;
      a2 = a3;
      std::cout << "log 3" << std::endl;
    }
    std::cout << "log 4" << std::endl;
  }
  std::cout << "Program finished!" << std::endl ;

}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ I cannot call this a shared pointer because the counter and destruction are not thread-safe. You cannot use it in multi-threaded context. \$\endgroup\$
    – ALX23z
    Mar 13, 2023 at 6:46
  • \$\begingroup\$ @ALX23z Shared pointers are useful in a single-threaded context as well. But you are right in the sense that std::shared_ptr has some level of thread-safety. \$\endgroup\$
    – G. Sliepen
    Mar 13, 2023 at 8:47
  • 3
    \$\begingroup\$ @G.Sliepen: Indeed; and it could be useful to implement a single_thread_shared_ptr for performance reasons, so ref counting can use cheap non-atomic operations, since ISO C++ doesn't have that; if you use shared_ptr, you get atomic RMW operations. So the problem here is the naming, not the lack of thread-safety. \$\endgroup\$ Mar 13, 2023 at 9:18

3 Answers 3

6
\$\begingroup\$

Overview

Self assignment is broken.

Code Review

First issue is here:

    my_shared_ptr(const my_shared_ptr& other) {

        // You are creating this object for the first time
        // So there is never going to be anything to destroy.
        destroy_current();
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        if (other.ptr_ != nullptr) {
            *counter_ += 1;
        }
    }

You really want move semantics to be exception safe.
I don't see any reason why it could not be exception safe.

    my_shared_ptr(my_shared_ptr&& other) {
    // So this function should be marked as noexcept.
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        
        // There is already a command to set and move (see: std::exchange)
        other.ptr_ = nullptr;
        other.counter_ = nullptr;
    }

This is actually broken.

Self assignment is going to release the memory, then increment the pointer on the newly released counter.

    my_shared_ptr& operator=(const my_shared_ptr& other) {
        destroy_current();
        
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        if (other.ptr_ != nullptr) {
            *counter_ += 1;
        }
    }

Try:

    my_shared_ptr<int>  first(new int{4});
    my_shared_ptr<int>& ref = first;

    first = ref; // bang.

Again. This should probably be exception safe. So mark it as such. Aslo you can use std::exchange() to do most of the work.

    my_shared_ptr&& operator=(my_shared_ptr&& other) {
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        
        other.ptr_ = nullptr;
        other.counter_ = nullptr;
    }

How I would write it:

template<typename T>
class my_shared_ptr
{
     T            ptr   = nullptr;
     std::size_t* size  = nullptr;


     bool decrementCountReachedZero() const
     {
         --(*size);
         return *size == 0;
     }

     public:
         ~my_shared_ptr()
         {
             if (size && decrementCountReachedZero()) {
                 delete ptr;
                 delete size;
             }
         }

         // Note: Default constructor
         //       Uses the default initialization values.
         my_shared_ptr() {}

         // Take ownership of a pointer.
         explicit my_shared_ptr(T* take)
             : ptr(take)
             , size(new std::size_t{1})
         {}

         // You may want a constructor that takes a `nullptr`
         // Because the above will not allow nullptr because of
         // the explicit
         my_shared_ptr(std::nullptr_t)
             : ptr(nullptr)
             , size(nullptr)
         {}

         // Standard Copy Constructor.
         my_shared_ptr(my_shared_ptr const& copy)
             : ptr(copy.ptr)
             , size(copy.size)
         {
             // OK we have constructed now.
             // So increment the size if it exists
             if (size) {
                ++(*size);
             }
         }

         // Standard Move Constructor
         my_shared_ptr(my_shared_ptr&& move) noexcept
             : ptr(std::exchange(move.ptr, nullptr))
             , size(std::exchange(move.size, nullptr))
         {
             // No action required
             // as original objects pointers have been set to null
             // Thus effectively transferring ownership of any
             // pointer.
         }


         // NOTE HERE
         //
         // I am writing both the copy and move assignment
         // operators out in full. This is NOT the best way
         // to do it. I am doing this to help illustrate the
         // correct way. (See below)

         // For normal copy assignment.
         // Use the copy and swap idiom.
         my_shared_ptr& operator=(my_shared_ptr const& copy)
         {
             my_shared_ptr tmp{copy};            // increment of any counters
                                                 // happens in the copy.
                                                 // decrement of any counters
                                                 // happens when this object
                                                 // is destroyed (which will not
                                                 // be the same as the object created :-)

             // Do the work.
             // Swap the tmp and the current object.
             // everything should be in the correct place.
             tmp.swap(*this);
             return *this;
         }
     
         // For normal move assignment.
         // Make sure the current object owned by "this" is destroyed.
         // Overwrite "this" one and set "move" old version to nullptr.
         my_shared_ptr& operator=(my_shared_ptr&& move) noexcept
         {
             my_shared_ptr tmp{std::move(*this)};// This moves the current object
                                                 // into a temp. This will be
                                                 // destroyed at end of scope
                                                 // and thus reduce the counter
                                                 // if there is one.


             // Do the work.
             // The current object is now null
             // So we swap with the source("move") thus putting nulls
             // into "move" and setting this object with the new value.
             move.swap(*this);
             return *this;
         }

         // NOTE: You will notice that both versions of
         //       the assignment operator are identical.
         // So there is a neat trick to simplify both these
         // into a single assignment operator that handles
         // both situations:

         // Simply pass by value.
         // If it is an lvalue it is copied.
         // If it is an rvalue it will be moved.
         // Which has the identical operation as the tmp variable
         my_shared_ptr& operator=(my_shared_ptr tmp) noexcept
         {
             tmp.swap(*this);
             return *this;
         }
         // At this point tmp goes out of scope.
         // destroying the tmp copy or the old value
        


         // Utility function.
         void swap(my_shared_ptr& other) noexcept
         {
             std::swap(ptr,  other.ptr);
             std::swap(size, other.size);
         }
         friend void swap(my_shared_ptr& lhs, my_shared_ptr& rhs)
         {
              lhs.swap(rhs);
         }
};
\$\endgroup\$
6
  • \$\begingroup\$ Great review (and I see I failed to make my answer's move constructor noexcept, though I remembered it on swap()). \$\endgroup\$ Mar 13, 2023 at 8:11
  • \$\begingroup\$ Great review. Thank you! \$\endgroup\$
    – mascai
    Mar 18, 2023 at 17:58
  • \$\begingroup\$ Are there advantages of using initializer list in constructor? \$\endgroup\$
    – mascai
    Mar 19, 2023 at 9:32
  • \$\begingroup\$ Why do you use temporary object in operator= ? \$\endgroup\$
    – mascai
    Mar 19, 2023 at 10:24
  • \$\begingroup\$ Are there advantages of using initializer list in constructor?: Yes. If you don't then objects will be defult constructed. So by default you should use them to form good habit. In C++ the most common way to change an object is to simply change the type so assume that it will have a default constructor at some point at you will always contruct the most effecient method by using initializer list. \$\endgroup\$ Mar 19, 2023 at 22:06
7
\$\begingroup\$

We're missing includes of <cstddef> and <utility>, and std::size_t is misspelt throughout.

Even with those fixed, the code isn't even valid, as neither operator= returns a value. They should both return *this as an lvalue-reference (not an rvalue-reference).

I'm not a big fan of using the trailing-underscore to differentiate member variables - if you have so many that a separate naming scheme is required, it's usually a sign that a class has multiple responsibilities.

The constructor that takes a raw pointer should be declared explicit, as implicit conversion could cause the target to be deleted when we didn't expect that.

Constructors should be using their initializer lists to populate data members, rather than default-initialising then assigning.

Copy-constructor, move-constructor and move-assignment are not exercised in the test program. Neither is the make_my_shared() function.

If I were writing this, I would use swap for the move constructor, and copy-and-swap to implement the assignment operators. That neatly avoids the bug where self-assignment resets the pointer to null.

    my_shared_ptr(my_shared_ptr&& other)
    {
        swap(other);
    }

    my_shared_ptr& operator=(my_shared_ptr other)
    {
        swap(other);
        return *this;
    }

    void swap(my_shared_ptr& other)
    {
        std::swap(ptr_, other.ptr_);
        std::swap(counter_, other.counter_);
    }

Instead of writing += 1 and -= 1, most C++ programmers would expect to see ++ and --. Similarly, explicit comparison to nullptr looks cluttered - often the sign of a Java programmer.

destroy_current is over-wordy. It can be simplified:

    void destroy_current()
    {
        if (counter_ && !--*counter_) {
            delete ptr_;
            delete counter_;
        }
    }

If that's too terse, then a middle ground might be

    void destroy_current()
    {
        if (!counter_) { return; } // null-pointer
        --*counter_;
        if (counter_ == 0) {
            // this was the last reference
            delete ptr_;
            delete counter_;
        }
    }

I don't like the way that destroy_current leaves the object in an invalid state (with pointers to deleted memory). That means that every use needs to be inspected to ensure it fixes the class invariant (i.e. those pointers are either both null or both point to valid objects). That said, once we use copy-and-swap and remove its pointless call from the copy constructor, it's only used in one location (the destructor), so we should just inline it there.

We're missing any support for array type as T, but nothing to prevent it either. If we choose to support arrays, we'll need to be use new[] and delete[].

It seems that no attempt has been made to consider thread safety in these methods - concurrent modification of other will result in unpredictable (possibly undefined) behaviour.


Modified code

#include <cstddef>
#include <utility>

// N.B. still not thread-safe
// T must be a simple object type - not array
template<typename T>
class my_shared_ptr
{
    T* ptr = nullptr;
    std::size_t* counter = nullptr;

public:
    my_shared_ptr()
    {}

    explicit my_shared_ptr(T* ptr)
        : ptr{ptr},
          counter{new std::size_t{1}}
    {
    }

    my_shared_ptr(const my_shared_ptr& other)
        : ptr{other.ptr},
          counter{other.counter}
    {
        if (counter) {
            ++*counter;
        }
    }

    my_shared_ptr(my_shared_ptr&& other)
    {
        swap(other);
    }

    my_shared_ptr& operator=(my_shared_ptr other)
    {
        swap(other);
        return *this;
    }

    ~my_shared_ptr()
    {
        if (counter && !--*counter) {
            delete ptr;
            delete counter;
        }
    }

    void swap(my_shared_ptr& other) noexcept
    {
        std::swap(ptr, other.ptr);
        std::swap(counter, other.counter);
    }
};
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Sure: explicit comparison to nullptr looks cluttered but this is overkill if (counter_ && !--*counter_). We should also remember the code should be easy to read. We don't want to be the next "Perl" \$\endgroup\$ Mar 13, 2023 at 0:51
  • \$\begingroup\$ Yes, we should be using the initializer lists in constructors - I'll fix that right now. \$\endgroup\$ Mar 13, 2023 at 7:52
  • \$\begingroup\$ I'll disagree on == nullptr being unidiomatic, and personally favors avoiding implicit boolean conversions, even in boolean contexts. \$\endgroup\$ Mar 13, 2023 at 8:29
  • 1
    \$\begingroup\$ @MatthieuM. I half agree with you. I find it situational weather you need the == nullptr. As long as the code is obvious (for me) to read I am happy. \$\endgroup\$ Mar 13, 2023 at 15:57
5
\$\begingroup\$

The unit test does not test the destructor and it should. I would suggest creating at least 1 function to implement the unit testing and call it from main(). To properly unit test you need to call every method implemented. It is quite possible the code does not work as you intend it to.

In destroy_current() what needs to be deleted is what ptr_ points to, not the fields ptr_ and counter_; it is possible that there is a memory leak here. It isn't clear why the destroy_current() function exists. The code in destroy_current() could easily be in the destructor itself.

The private variable counter_ should not be a pointer to std::size_t: it should be a std::size_t. There is no reason to allocate the pointer; it will be allocated by the constructor by default.

You need to become comfortable with initialization lists in the constructors, as most of the constructors contain unnecessary code. Instead of the following:

    my_shared_ptr(T* ptr) {
        ptr_ = ptr;
        counter_ = new size_t(1);
    }

    my_shared_ptr(const my_shared_ptr& other) {
        destroy_current();
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        if (other.ptr_ != nullptr) {
            *counter_ += 1;
        }
    }

    my_shared_ptr(my_shared_ptr&& other) {
        ptr_ = other.ptr_;
        counter_ = other.counter_;
        
        other.ptr_ = nullptr;
        other.counter_ = nullptr;
    }

The code could be:

    my_shared_ptr(T* ptr)
    : ptr_ { ptr }, counter_ { 1 }
    {
    }

    my_shared_ptr(const my_shared_ptr& other)
    : ptr_ { other.ptr }, counter_ { other.counter_ }
    {
        if (other.ptr_ != nullptr) {
            *counter_ += 1;
        }
    }

    my_shared_ptr(my_shared_ptr&& other)
    : ptr_ { other.ptr }, counter_ = { other_counter_ }
    {
        other.ptr_ = nullptr;
        other.counter_ = nullptr;
    }

Use comments to identify the copy constructor and the move constructor. Because counter_ is a pointer, the move constructor is keeping a link to the original counter_ variable rather than updating the count.

\$\endgroup\$
4
  • 3
    \$\begingroup\$ I believe that counter is a pointer so that copies share the counter of the original. \$\endgroup\$ Mar 12, 2023 at 15:39
  • \$\begingroup\$ @G.Sliepen I never liked destroy_current(), is this better? \$\endgroup\$
    – pacmaninbw
    Mar 12, 2023 at 22:37
  • \$\begingroup\$ Yes, that is better. \$\endgroup\$
    – G. Sliepen
    Mar 12, 2023 at 23:41
  • 3
    \$\begingroup\$ The private variable counter_ should not be a pointer to std::size_t . It needs to be a pointer. \$\endgroup\$ Mar 13, 2023 at 1:00

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.