7
\$\begingroup\$

I've written an implementation of shared_ptr. It doesn't support weak_ptr (yet) but below is the code. I'd appreciate any feedback, comments.

#pragma once

#include <iostream>
#include <memory>
#include <utility>
#include <cxxabi.h>


template <typename T>
void del(T* t)
{
   delete t;
};

namespace graham
{

   using namespace std;
   struct control_block_base 
   {
      template <typename T, typename Deleter>
      control_block_base(T* t, const Deleter& d) 
      {
         _cleanup = [=] () 
         { 
            const Deleter& _d= std::move(d);
            _d(t); 
            // TODO only delete this is weak_ptr count == 0
            delete this;
         };

         _deleter = (void*) &d;

         // only for debugging/operator<<
         int status_ignored;
         _true_type = abi::__cxa_demangle(typeid(T).name(),NULL,NULL,&status_ignored);
      };

      virtual ~control_block_base()
      {
      }

      void reset()
      {
         dec();

      }

      void dec()
      {
         if (!--_ref_count)
         {
            _cleanup();
         }
      }

      void* get_deleter()
      {
         return _deleter;
      };

      std::string _true_type;
      std::atomic<int> _ref_count;
      void* _deleter;
   private:

      function<void()> _cleanup;
   };

   template <typename T>
   struct control_block   : public control_block_base
   {

      control_block(nullptr_t ) : _ptr{nullptr}
      {
         this->_ref_count = 0;
      }


      control_block(T* ptr) : control_block_base(ptr, std::default_delete<T>()) ,
      _ptr{ptr} 
      {
         this-> _ref_count = 1;
      };

      template <typename U, typename Deleter>
      control_block(U* ptr, const Deleter& d) : control_block_base(ptr, std::move(d)) ,
      _ptr{ptr} 
      {
         this-> _ref_count = 1;
      };

      ~control_block()
      {
      }

      T* get()
      {
         return _ptr;
      }

      T* _ptr;

   };

   template <typename T> 
   class shared_ptr
   {
      using element_type = T;

      template <typename>
      friend class shared_ptr;


   public:

 ~shared_ptr()
      {
         if (_cb)
         {
            _cb->dec();
         }
      } 

      constexpr shared_ptr() noexcept : _ptr(nullptr), _cb(nullptr) 
      {
      } 

      constexpr shared_ptr(std::nullptr_t ptr) noexcept : _cb(nullptr), _ptr(nullptr) 
      {
      }

      explicit shared_ptr(T* ptr) : _ptr(ptr), _cb (new control_block<T>(ptr))
      {
      }

      shared_ptr(const shared_ptr<T>& other)
      {
         if (( _cb = other._cb))
         {
            ++_cb->_ref_count;
         }
      }

      template <typename Deleter>
      shared_ptr(T* ptr, Deleter d) : _ptr(ptr), _cb(new control_block<T>(ptr, d))
      {
      }

      template <typename U>
      explicit shared_ptr(U* ptr) : _ptr(ptr),
      _cb (new control_block<U>(ptr, std::default_delete<U>()))
      {
      }

      template <typename U, typename Deleter>
      shared_ptr(U* ptr, Deleter d) :  _ptr(ptr),
      _cb (new control_block<U>(ptr, d))
      {
      }


      template <typename U>
      shared_ptr(shared_ptr<U>& u, element_type* e) : _ptr(e), _cb(u._cb)
      {  
          if (_cb)
          {
             ++_cb->_ref_count;
          }
      };

      template <typename U>
      shared_ptr(const shared_ptr<U>& u) : _ptr(u._ptr), _cb(u._cb)
      {
         if (_cb)
          {
             ++_cb->_ref_count;
          }
      };

      template <typename U, typename D>
      shared_ptr(std::unique_ptr<U,D>&& up) : _cb(new control_block<U>(up.get(), up.get_deleter()))
      {
      }


      template <typename U>
      bool operator==(const shared_ptr<U>& other) const
      {
         return _ptr == other._ptr;
      }

      template <typename U>
      bool operator!=(const shared_ptr<U>& other) const
      {
         return !(*this == other);
      }

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

      T* operator->() const noexcept
      {
         return _ptr;
      }


      T& operator*() const noexcept
      {
         return *_ptr;
      }

      void swap(shared_ptr& other)
      {
         std::swap(_cb, other._cb);
         std::swap(_ptr, other._ptr);
      }

      shared_ptr& operator=(const shared_ptr& other) 
      {
         shared_ptr tmp(other);
         swap(tmp);
         return *this;
      }

      // Assignment, copy swap idiom
      template <typename U>
      shared_ptr<T>& operator=(const shared_ptr<U>& other)
      {
         shared_ptr temp(other);
         this->swap(temp);
         return *this;
      }

      template <typename U>
      shared_ptr<T>& operator=(shared_ptr<U>&& other)
      {
         shared_ptr temp(std::move(other));
         this->swap(temp);
         return *this;
      }


      T* get() const noexcept
      {
         return _ptr;
      }

      void reset()
      {
         if (_cb)
         {
            _cb->dec();
         }
      }

    // why is return value not unsigned?
      long use_count() const
      {
         return (_cb ? _cb->_ref_count.load() : 0);
      }


      template <typename U>
      friend std::ostream& operator<<(std::ostream& os, shared_ptr<U>& sp);

   private:

      T* _ptr = nullptr;
      control_block_base *_cb = nullptr;

   };


   // This is merely for debugging, useful though.
   template <typename U>
   std::ostream& operator<<(std::ostream& os, shared_ptr<U>& sp)
   {

      os << "use_count: ";
      if (sp._cb)
      {
         int status_ignored;
         os << sp.use_count() 
         << ",  shared_ptr<T="
         << abi::__cxa_demangle(typeid(*(static_cast<control_block<U>*>((sp._cb))->_ptr)).name(),NULL,NULL,&status_ignored)
         << ", U=" 
         << sp._cb->_true_type
         << "> , _ptr="
         << sp._ptr;
      }
      else
      {
         os << "nullptr/empty";
      }
      return os;
   };

   template <typename Deleter, typename U>
   Deleter* get_deleter(const shared_ptr<U>& up)
   {
      void* deleter = nullptr;
      return (Deleter*) deleter;
   }
}

And here is a main with some dummy test classes (base and derived). The destructor for base is deliberately non virtual to test the type erasure (deleter in control block).

Feel free to comment, its a first implementation.

#include <iostream>
#include <memory>
#include <ostream>
#include "shared_ptr.h"
#include <cassert>


using namespace std;


int base_counter = 0;
int derived_counter = 0;

struct base
{

   // non virtual, deliberately, shared_ptr's deleter will delete in terms of the correct type, T or U
   ~base()
   {
      //cout << "                                 base::~base() " << endl;
      --base_counter;
   }

   base()
   {
      // cout << "base::base() " << endl;
      ++base_counter;
   }

   base(int i) : _i(i)
   {
      ++base_counter;
      // cout << "base::base() " << endl;
   }

   void foo()
   {
      // cout  << "base::foo() " << endl;
   }

   int get()
   {
      return _i;
   }

   int _i;
};


struct derived : public base
{
   derived()
   {
      // cout << "derived::derived() " << endl;
      ++derived_counter;
   };

   derived(int i) : base(i)
   {
      ++derived_counter;
      // cout << "derived::derived() " << endl;
   };

   // non virtual
   ~derived()
   {
      --derived_counter;
      // cout << "                                 derived::~derived() " << endl;
   };

   void foo()
   {
      // cout  << "derived::foo() " << endl;
   }
};


void delit(derived* d)
{
   //cout << "-------------> delit(derived*) called " << endl;
   delete d;
}

void dbase(base* b)
{
   //cout << "-------------> delit(base*) called " << endl;
   delete b;
}

int main()
{
   int i = 0;
   assert(derived_counter==0 && base_counter == 0);
   cout << endl << endl;

   {
      cout << "TEST " << i++ << " ";
      graham::shared_ptr<base> sp;
      cout << "sp: " << sp << endl;
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      graham::shared_ptr<base> bp (new base());
      bp.reset();
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << " " ;
      graham::shared_ptr<base> bp (new base(), &dbase);
      using Deleter = decltype(dbase);
      Deleter* deleter = graham::get_deleter<Deleter, base>(bp);
      int status;
      std::string realname = abi::__cxa_demangle(typeid(deleter).name(), NULL, NULL, &status);
      cout << "Deleter: " << realname << endl;
      bp.reset();
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << " " ;
      graham::shared_ptr<base> bp2 (new derived(), delit);
      assert(derived_counter==1 && base_counter == 1);
      cout << "bp2: " << bp2 << endl;
      bp2.reset();
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp2 (new derived(), [] (derived* b){ delete b;});
      bp2.reset();
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      // ~base() is not virtual, hence derived will not be destructed, deliberate
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp2 (new derived(), [] (base* b){ delete b;});
      bp2.reset();
   }
   assert(derived_counter==1 && base_counter == 0);
   --derived_counter;

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp2 (new derived(), [] (derived* d){ delete d;});
      bp2.reset();
      assert(derived_counter==0 && base_counter == 0);
   }

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp2 (new derived());
      bp2.reset();
      assert(derived_counter==0 && base_counter == 0);
   }

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp2 (new derived());
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST (alias ctor) " << i++ << endl;
      base* bp2 = new base();
      base* bp3 = new base();
      {
         graham::shared_ptr<base> bsp (bp2 );
         graham::shared_ptr<base> bsp2 (bsp,bp3 );
      }
      assert(derived_counter==0 && base_counter == 1);
      delete bp3;
      assert(derived_counter==0 && base_counter == 0);
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp;
      graham::shared_ptr<base> bp2(bp);
      graham::shared_ptr<base> bp3(new base());
      bp = bp3;
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      shared_ptr<base> bp(new derived());
      shared_ptr<base> bp2(bp);
      assert(bp == bp2);
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      shared_ptr<base> bp(nullptr);
      shared_ptr<base> bp2(bp);
      assert(bp2.get() == nullptr);
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp; 
      graham::shared_ptr<base> bp2(bp);
      assert(bp == bp2);
   }
   assert(derived_counter==0 && base_counter == 0);


   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<derived> bp(new derived()); 
      graham::shared_ptr<base> bp2(bp);
      assert(bp == bp2);
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<derived> bp(new derived()); 
      graham::shared_ptr<base> bp2;
      bp2 = bp;
      assert(bp2 == bp);
   }

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<derived> bp(new derived(3)); 
      graham::shared_ptr<base> bp2;

      bp2 = std::move(bp);
      assert(bp2->_i == 3);
      
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<derived> bp;
      assert(bp.get() == nullptr);

      graham::shared_ptr<derived> bp2(new derived());
      if (!bp2)
      {
         assert(false);
      }
      
   }

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<derived> bp(new derived());
      graham::shared_ptr<base> bp2;
      bp2 = bp;
      graham::shared_ptr<base> bp3(new derived());
      graham::shared_ptr<base> bp4(new derived());
      graham::shared_ptr<base> bp5(new derived());
      graham::shared_ptr<base> bp6(new derived());
      graham::shared_ptr<base> bp7(new base());
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<derived> bp (new derived());
      graham::shared_ptr<base> bp1;
      graham::shared_ptr<base> bp2;
      graham::shared_ptr<base> bp3;
      graham::shared_ptr<base> bp4;
      bp1 = bp;
      bp2 = bp1;
      bp3 = bp2;
      bp4 = bp3;
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<derived> dp (new derived());
      graham::shared_ptr<derived> dp2 (new derived());
      graham::shared_ptr<base> bp1(new base());
      graham::shared_ptr<base> bp2(new base());
      graham::shared_ptr<base> bp3(new base());
      graham::shared_ptr<base> bp4(new base());
   }
   assert(derived_counter==0 && base_counter == 0);

   {
      cout << "TEST " << i++ << endl;
      graham::shared_ptr<base> bp(new base(123));
      graham::shared_ptr<base> bp2(new base(321));
      bp.swap(bp2);
      assert(bp->get() == 321);
      assert(bp2->get() == 123);
   }
   assert(derived_counter==0 && base_counter == 0);



   return 0;

}
\$\endgroup\$
5
  • \$\begingroup\$ Is there a particular reason that you don't just use the C++ STL? \$\endgroup\$ Jul 20, 2023 at 17:11
  • \$\begingroup\$ What's <cxxabi.h>? That's not a standard C++ header. \$\endgroup\$ Jul 20, 2023 at 20:13
  • \$\begingroup\$ Its my implementation, learning purposes. cxxabi.h is for demangling/trace/debug info, as commented. // only for debugging/operator<< Any useful critiques out there? \$\endgroup\$
    – Greg
    Jul 20, 2023 at 20:51
  • \$\begingroup\$ @FreezePhoenix, can you elaborate on how you would use STL please? Thanks. \$\endgroup\$
    – Greg
    Jul 21, 2023 at 11:03
  • \$\begingroup\$ @Greg The STL (Standard Template Library) for C++ already contains an implementation of shared_ptr and unique_ptr. \$\endgroup\$ Jul 21, 2023 at 15:36

2 Answers 2

3
\$\begingroup\$

This Looks Pretty Solid

You increment and decrement the count as an atomic variable, use swap idioms to implement copy and assignment, re-use code by delegating constructors, and generally implement the Standard Library API. The implementation has a basic control-block interface that’s independent of the type and implementations for each type.

However, I do have some advice, starting with:

Check for Logic Errors (Statically if Possible)

Currently, your code to decrement and free a control block is:

      void dec()
      {
         if (!--_ref_count)
         {
            _cleanup();
         }
      }

Where _ref_count is defined as a std::atomic<int>. So, the operation here that’s actually atomic is subtracting from the counter, not the entire call to control_block_base::dec.

If the program logic correctly ensures that dec is called no more than once per shared pointer that exists, which are not enough to overflow the counter, this will work. However, it would be better to detect logic errors immediately and robustly. The advantage of a signed atomic counter is that going out of range produces a negative value, (In C++23 or with atomic types even before that, overflowing the counter is guaranteed to produce two’s-complement wraparound, not undefined behavior.) I therefore recommend that both inc and dec check for a negative count, and either abort or throw an exception if it detects wraparound.

That isn’t necessarily an unrealistic scenario: on some platforms I’ve actually coded for (such as MS-DOS or 16-bit Windows running on an 80386 or higher), an int overflows after a value of only 32,767, but there’s enough memory to hold more copies of a shared pointer than that, and the hardware is capable of 32-bit atomic increments and decrements. It’s harder to imagine an algorithm that needs to create more than two billion copies of the same shared pointer (Filling some huge data structure with shared copies of the same value?), but it could happen and you should detect it if it does.

I therefore recommend you make the counter an atomic_intptr_t. On every platform I can think of, this is able to count all the pointers that could actually fit in memory, and also lock-free.

Hide Helpers that Aren’t Part of the API

You can nest a class inside another class, so you declare a helper class within the private section of shared_ptr<T>. That way, it’s an encapsulated implementation detail that no one but the class itself can see. (If derived classes should be able to see it also, make it protected instead.) This would be a good spot to encapsulate control_block.

Another good place to stick things like this is in an inner namespace. If you declare something like namespace details inside namespace graham, you now can refer to graham::details::foo, and this will not be brought into the global namespace when someone uses namespace graham. it’s also an explicit way to tell client code, anything within the inner namespace isn’t supposed to be used, and might change.

Refactor Your Classes to Use Composition

There isn’t one unambiguously best design, but the one here has some disadvantages. A major one is that it promiscuously allows all assignments of shared_ptr<T> to shared_ptr<U> and stores the data behind a void*, plus some non-portable G++ extensions for real-time type information, which it might check but often doesn’t. Runtime reflection like this is slower and more error-prone, and you should if possible use static type-checking to ensure type safety at compile time. (In this instance, you might have done that for the sake of get_deleter, and I’ll come back to that later).

Another sign that you’re drawing the line of encapsulation in the wrong place is that your classes constantly need to be friends of each other need to see each other’s private details (not just the different specializations of shared_ptr<T> and shared_ptr<U>, which is unavoidable with this interface). For example, control_block_base<T>::dec() gets called from ~shared_ptr<T>, which also needs to do cleanup that control_block_base doesn’t know about. If you ever add any code to this destructor that needs to be part of the same atomic operation, you would need to refactor. Additionally, you expose the raw counter itself to shared_ptr and have it alter the value directly. And, finally, every place dec() gets called is either ~shared_ptr<T>, or could be replaced by something that invokes the destructor, such as implementing shared_ptr<T>::reset() with swap(graham::shared_ptr{});. So, dec() is part of the wrong class, and its code should just be folded into ~shared_ptr().

In fact, you might not even need your helper classes at all. You derive a family control_block<T, Deleter> from control_block_base<T>, but control_block_base<T> already contains a std::function object that gets invoked to delete the object, so this could simply become the deleter and replace the templated subclasses. You also don’t need to wrap it in a lambda; if the deleter is invokable, you can initialize your std::function from d directly. (Unless you need get_deleter to work as intended, which has some very high overhead for some very rare use cases. I’ll come back to that.)

If you enforce type safety on copying, so that you can only represent a shared_ptr<U> as a shared_ptr<T> if T is layout-compatible with U or a pointer-interconvertible superclass of U, all copies can additionally share a single pointer and deleter. These can be go into control_block<T>, which could become a simple struct (or you might choose to move the data pointer into the shared_ptr instance on the theory that this saves you a lot of double-dereferencing).

However, you might need to support conversions that are not pointer-interchangeable. You do not want to have to do at least one, and possibly a chain of, virtual function calls every time you want to obtain a pointer of the correct type. Dereferencing the pointer is a the most common and important operation you do on a shared_ptr, while a destructor on a converted pointer will run either never or once. So, you definitely want to optimize for the common operation, not the deleter.

Therefore, if you are not going to restrict conversions to pointer-interconvertible types, each shared_ptr<T> should hold its own T*. Converting back to the original pointer type that the deleter expects is impossible in the general case. The deleter should therefore be a std::function<void()> in the control block, holding the closure std::bind(d, data_ptr) (which is similar to what you do with the lambda _cleanup).

It would additionally be possible to extend that scheme to perform a dynamic_cast that can possibly fail at runtime, such as from shared_ptr<Base> to shared_ptr<Derived> when you know the pointer is actually to a Derived. I recommend, if you want to take that route, that you add an overload that checks the typeid of the data pointer (which must be polymorphic) against the requested type, and throws a std::runtime_error if they are incompatible, or represent failure as the magic default value.

Use restrict and Type Constraints to Limit Your Overloads

Right now, you declare a rather promiscuous interface and defer most of the type checking to runtime. It would be much better to catch type errors at compile time. For example, you can catch whether the second arguments to your constructors are, in fact, deleters, with a helper concept like this one:

template<class F, class T>
concept is_functor_of =
    std::is_nothrow_convertible_v<F, std::function<void(T*)>>;

It might be even simpler to use std::invocable. The nothrow restriction, which also is part of the std::shared_ptr interface, allows functions that pack the deleter into a std::function to be noexcept. if you additionally check that the deleter never throws, you could make functions that destroy shared_ptr objects noexcept, too.

Other useful type-traits for this project include std::is_layout_compatible and std::is_pointer_interconvertible_base_of. These will let you write overloads of the conversion operators that support trivial conversions, safely and with zero overhead.

You already implement all shared_ptr<U> to shared_ptr<T> conversions as copy or move constructors,which is a good idea, but you create a new control block for each, which then attempt to share the same control_block_base. This is unnecessarily complex.

Use Non-Member Functions When Appropriate

Best practice is for binary operators where objects of the class could appear on either side to be non-member functions, so that overload resolution works correctly.

An example of this is if you want to make false == ptr work like false == (bool)ptr. The operator overload you need must be a non-member function, since the class instance is on the right. And in that case, you also want to provide a matching overload witj shared_ptr om the left and bool on the right. This should also be a non-member function, for consistency. The compiler will then automatically fill in the ! and != operators for you.

Unfortunately, if you also need them to be a friend, there’s a four-step process. First, you write the non-member function after the class declaration, so that it can see the member functions. Then, you declare the non-member functions friend within the class. They should be declared graham::operator== in this context, to avoid confusion with other possible overloads. But then, if you want them to be static, inline or constexpr, you need forward declarations before the class so the compiler can find them. Finally, to be able to write those declaraations, you need an incomplete declaration of the class.

In this case, that led me to put this immediately before the real definition of shared_ptr:

// Need a bit of forward declaration for friend functions to work:
template <typename T>
class shared_ptr;

template <typename T>
constexpr void swap(my::shared_ptr<T>& x,
                    my::shared_ptr<T>& y) noexcept;

template<typename U, typename V>
constexpr bool operator==(const my::shared_ptr<U>&, const my::shared_ptr<V>&) noexcept;

Alternatively, you can often write non-member functions using only the public interface. For example, if you return actual references to the data and have some way to test whether the pointer holds valid data, you can determine and compare the addresses of the data using public member functions.

Some of Your Implementations Can be Improved

One thing jumps out at me as a missed opportunity. Currently, you have:

      template <typename U>
      shared_ptr<T>& operator=(shared_ptr<U>&& other)
      {
         shared_ptr temp(std::move(other));
         this->swap(temp);
         return *this;
      }

The overload makes sense if you’ll be doing move optimization, to optimize away the increments and decrements (since you know in advance that the reference count will be the same). However, you don’t appear to have an optimized move constructor that skips incrementing and decrementing. A lot of other places where this optimization could work therefore don’t, such as:

shared_ptr temp(std::move(other));

While you’re here, the intermediate copy to temp and this-> can both be removed, getting you the slightly shorter

     swap(std::move(other));
     return *this;

The swap function should really be declared as a non-member function, in the same namespace as the objects you’re swapping. One optimization is that a swap between two T&& can become a no-op, since moving the operands leaves them both in some valid but indeterminate state, and their initial state qualifies.

Don’t Import All of namespace std, Even Inside Another Namespace

In this case, it’s especially dangerous because you #include <memory>, which means you have two shared_ptr<T> class templates in the same namespace. You might survive that if you are very, very careful about always writing graham:: in front of shared_ptr, but chances are, we’re all going to slip up some of the time.

The reason std:: was created in the first place is that, otherwise, it would be impossible to write a forward-compatible program, because there would be no way to know in advance what identifiers someone might add to a future version of the library. If you just bring in the whole namespace, you’re right back in that situation where your code can break. Admittedly, this is a lost cause: enough programmers did write using namespace std; that the Standards Committee now tries to avoid breaking it by putting new identifiers inside a second level of namespaces within std.

About That One Comment

I’m thinking of

// TODO only delete this is weak_ptr count == 0

This is misguided, in my opinion, although this could work properly if it applies only to the control block itself. The classic example is, to implement circular references, which otherwise would leak when there are no external references to it. Hence this “hacker koan” from the Jargon File:

One day a student came to Moon and said: "I understand how to make a better garbage collector. We must keep a reference count of the pointers to each cons."

Moon patiently told the student the following story:

"One day a student came to Moon and said: `I understand how to make a better garbage collector...

If you do go this route, since you are using an atomic reference counter, the strong and weak reference counters can no longer be incremented, but must be updated together in a CAS loop, or a thread might see them in an inconsistent state. On x86-64 targets with the cmpxchg16b instruction, major compilers will generate lock-free atomic code to update two int64_t values at once, but only (last I tested) if they are in a struct declared alignof(16).

So, in general, your best bet to get two counters working in a thread-safe way is to pack them in a naturally-aligned struct, wrap that in std::atomic, and write a loop with atomic_compare_exchange_strong. You might want to static_assert that the operations are always lock-free, and use smaller counters if not.

What if You Need shared_ptr::get_deleter?

The simplest way to implement this is to always return a null pointer, which technically checks off all the requirements.

If you want to make a genuine best effort, the control block should keep a copy of the original deleter. One portable way to implement the runtime lookup for non-polymorphic objects would be to have the statically-resolved get_deleter<T> call some virtual function like virtual const void* control_block<T, Deleter>::get_deleter(const std::type_info&), which can compare the std::type_info object to typeid(Deleter) at runtime.

If you’re going to use compiler specific extensions, which aren’t necessary in this case, you should put them inside conditional blocks, such as #if __GNUC__.

Some Other Minor Points

Best practice is to use #ifndef SHARED_PTR_H/#define SHARED_PTR_H instead of (or in addition to) the nonstandard #pragma once.

You have several instances of this->, which should normally be unnecessary. (Avoid creating name collisions that would make you disambiguate.)

You should mark functions noexcept where appropriate. For example, swap and all functions that can be implemented with it can be noexcept.

As a side note, if your shared_ptr objects end up holding just two pointers (to the data of the requested type, and the control block), you can improve performance on some architectures by naturally-aligning the class to alignas(2*sizeof(void*)). This is because the CPU can then use larger, aligned loads and stores to move them around, and the data will never be split across two fetches or cache lines. This is also how you are expected to implement the performance guarantees for std::atomic<shared_ptr<T>>.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ THIS is EXACTLY what I was hoping for. Thank you VERY much @Davisior. I've enough there to get stuck into. A bit of reading too. Some silly things I should have known, too (didn't realise #pragma is non standard), using namespace std among other silly stuff. I COMPLETELY overloooked concepts. Right better get busy. Thank you SO much. I'll start by writing tests to reproduce errors, correct and retest etc. Thanks a million !!! \$\endgroup\$ Jul 22, 2023 at 16:40
1
\$\begingroup\$

I'll write what I found that wasn't covered by @Davislor. You have some issues with the code

     _cleanup = [=] () 
     { 
        const Deleter& _d= std::move(d);
        _d(t); 
        // TODO only delete this is weak_ptr count == 0
        delete this;
     };

To me, calling delete this inside lambda that captures by = seemed like a bug but apparently it is not... only that it is deprecated (was awful choice of syntax from the start IMHO). Always capture this either by copy or reference manually to avoid confusion.

The call const Deleter& _d= std::move(d); is redundant. It doesn't even perform a copy nor extends lifetime in anyway if that's what you were worried about.

virtual ~control_block_base()
  {
  }

It is better to write virtual ~control_block_base() = default;. Perhaps it has lesser value when the call is virtual, still opt to =default instead of writing an empty function definition. Also remove the redundant definition ~control_block(){}.

API and Design

You lack aliasing constructor. See version (8) from https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr it is one of the important pros of the control-block based shared pointers. The idea is that you point towards one object while managing another. This way one can supply some function a shared pointer towards a member of the object while managing the whole object's lifetime via the same control block.

And as mentioned by @Davislor, you lack proper move-constructors. They are important.

Also add variety of implicit casting constructors when it is safe and functionality for dynamic_casting and static_casting at upcasting and other pointer casting.

I don't understand why you need to have a base class control_block_base and template control_block. I am certain that a single class can do it just as easy and you don't need to make it into a polymorphic one.

Atomics

There are some things I disagree with @Davislor. There is no reason to check negatives on refcount or reaching zero twice or anything of the sort. The only way this could have ever happened in the first place is when user has already created several bad data races in the code. It's already unsalvageable situation so you don't need to bother with it.

You don't use the atomic operations right. You call standard operations like ++/--/= all of which are slow. It is a way to signal that you don't know what you are doing. In general, atomic instructions are used to synchronize variables that aren't a part of the atomic variable and thus calling them unnecessarily can lead to severe slowdown in some situations. By default, the operations you call on atomic enforce the strongest fence available which is far from ideal.

On increment you should increment it via relaxed instruction (fetch_add(1, std::memory_order_relaxed)). On decrement you ought to call release as to ensure that the deleting call has the modified data (fetch_sub(1, std::memory_order_release)). While right before deletion, you ought to call an instruction with acquire to load whatever changes that could've happened (say, load(std::memory_order_acquire) and discard output).

\$\endgroup\$
16
  • 1
    \$\begingroup\$ this is also excellent! These 2 reviews (yours and previous) are EXACTLY what I wanted from this code review. Thank you \$\endgroup\$ Jul 22, 2023 at 21:00
  • \$\begingroup\$ This too is extremely valuable. Both yourself and @Davislor serve as an example to what stackoverflow should be. No noise, insightful and educated. Thank you both very much. Appreciate your time. "namespace \$\endgroup\$ Jul 22, 2023 at 21:35
  • \$\begingroup\$ Good to see a second opinion. To clarify about checking for logic errors: I agree that a negative value unrecoverable. The point of checking for them is to crash immediately and tell the maintainer exactly what went wrong. If you let the program keep running and decrementing its negative ref count, it might cause a bug later that’s hard to reproduce, or more likely, will appear to run correctly but have a memory leak that’s very hard to find. And the cost of checking is absolutely negligible. \$\endgroup\$
    – Davislor
    Jul 24, 2023 at 19:36
  • \$\begingroup\$ I also brought up that there should be aliasing and converting constructors, and that the control_block class only needs to be templated if you need to implement shared_ptr::get_deleter. (Making the template parameter the type of the stored deleter would also allow refactoring to remove _cleanup.) These are important points, and it looks like they could’ve got lost in my big answer, so thanks for bringing them up here. \$\endgroup\$
    – Davislor
    Jul 24, 2023 at 19:46
  • \$\begingroup\$ @Davislor once the refcount reached zero then the control block gets deleted. I have some doubts that even the checking mechanism will be able to function past that. Or do you refer to the scenario when a weak_ptr maintains it? \$\endgroup\$
    – ALX23z
    Jul 25, 2023 at 2:55

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.