Improvement suggestions to my shared_ptr and weak_ptr implementations

Question

Below is the entire code. I appreciate if, someone with more C++ experience can suggest if this can be improved further and if you notice any issues.

controlblock.hpp

template<class T>
struct ControlBlock
{
    int _ref_count;
    int _weak_count;
    void inc_ref() noexcept { ++_ref_count; }
    void inc_wref() noexcept { ++_weak_count; }
    void dec_ref() noexcept { --_ref_count; }
    void dec_wref() noexcept { if (--_weak_count == 0) delete this; }
    int use_count() const noexcept { return _ref_count; }
    ControlBlock() : _ref_count{ 0 }, _weak_count{ 0 } {}
    ~ControlBlock() { _ref_count = 0; _weak_count = 0; }
};

shared_ptr.hpp

#pragma once

#include <algorithm>
#include <iterator>
#include <compare>
#include <cassert>
#include <memory>

#include "controlblock.hpp"
#include "weak_ptr.hpp"

template<class T>
class SharedPtr {
#define CHECK assert(Invariant());
    template<class Y> friend class WeakPtr;
private:
    T* _ptr;
    ControlBlock<T>* _ctrl_block;

    void __increment_reference()
    {
        if (_ptr != nullptr && _ctrl_block != nullptr)
                _ctrl_block->inc_ref();
    }

    void __remove_reference()
    {
        if (_ctrl_block && _ctrl_block->_ref_count > 0) {
            --_ctrl_block->_ref_count;
            if(_ctrl_block && _ctrl_block->_ref_count == 0){
                delete _ptr;
            }
            
            if(_ctrl_block->_ref_count + _ctrl_block->_weak_count == 0) {
                delete _ctrl_block;
            }
            
            _ptr = nullptr;
            _ctrl_block = nullptr;
        }
    }

public:
    constexpr SharedPtr() : _ptr{ nullptr }, _ctrl_block{ nullptr } { }
    explicit SharedPtr(T* other) : _ptr{ other }, _ctrl_block{new ControlBlock<T>}
    {
        __increment_reference();
        CHECK 
    }
    constexpr SharedPtr(const std::nullptr_t) noexcept 
        : _ptr{ nullptr }, _ctrl_block{ nullptr } { CHECK }
    
    explicit SharedPtr(WeakPtr<T>& other)
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    {
        if (other.expired())
            throw std::bad_weak_ptr();
        
        _ctrl_block->inc_wref();
        CHECK
    }

    SharedPtr(const SharedPtr& other) noexcept 
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    {
        __increment_reference();
        CHECK
    }

    SharedPtr(SharedPtr&& other) noexcept 
        : _ptr{ std::exchange(other._ptr, nullptr) }, 
          _ctrl_block{std::exchange(other._ctrl_block, nullptr)} 
    { 
        CHECK 
    }
    
    ~SharedPtr() { __remove_reference(); CHECK };

    SharedPtr& operator=(const SharedPtr& other) noexcept 
    {
        if (this == &other)
            return *this;

        __remove_reference();
        _ptr = other._ptr;
        _ctrl_block = other._ctrl_block;
        __increment_reference();

        CHECK
        return *this;
    }

    SharedPtr& operator=(SharedPtr&& other) noexcept
    {
        if (this == &other)
            return *this;

        __remove_reference();
        _ptr = std::exchange(other._ptr, nullptr);
        _ctrl_block = std::exchange(other._ctrl_block, nullptr);

        CHECK 
        return *this;
    }

    SharedPtr& operator=(std::nullptr_t)
    {
        if (_ptr == nullptr && _ctrl_block == nullptr)
            return *this;

        __remove_reference();
        return *this;
    }

    T* get() const noexcept { return _ptr; }
    T& operator*() const noexcept { return *_ptr; }
    T* operator->() const noexcept { return _ptr; }
    operator bool() const noexcept { return _ptr != nullptr && _ctrl_block != nullptr; }

    size_t use_count() const noexcept { return (_ctrl_block) ? _ctrl_block->use_count() : 0; }
    
    void reset() noexcept { this->__remove_reference(); }
    bool Invariant() const noexcept;

    template<class T> friend void swap(SharedPtr<T>& lhs, SharedPtr<T>& rhs) noexcept;

    friend auto operator<=>(const SharedPtr& lhs, const SharedPtr& rhs) = default;
    friend auto operator==(const SharedPtr& lhs, const SharedPtr& rhs) 
    {
        if (lhs.get() != rhs.get())
            return false;

        return (lhs.get() <=> rhs.get()) == 0;
    }
};

template<class T>
inline bool SharedPtr<T>::Invariant() const noexcept
{
    if (_ptr == nullptr && _ctrl_block == nullptr)
        return true;
    else if (_ptr != nullptr || _ctrl_block != nullptr && 
           _ctrl_block->_ref_count > 0 || _ctrl_block->_weak_count > 0)
        return true;
    
    return false;
}

template<class T> void swap(SharedPtr<T>& lhs, SharedPtr<T>& rhs) noexcept 
{
    std::swap(lhs, rhs);
}

template<class T, class ...Args>
SharedPtr<T> MakeShared(Args && ...args)
{
    return SharedPtr<T>(new T(std::forward<Args>(args)...));
}

weak_ptr.hpp

#pragma once

#include "controlblock.hpp"
#include "shared_ptr.hpp"

template<class T>
class WeakPtr {
#define CHECK assert(Invariant());
    template<class Y> friend class SharedPtr;
private:
    T* _ptr;
    ControlBlock<T>* _ctrl_block;
public:
    WeakPtr() { CHECK }
    WeakPtr(T* other) : _ptr{other}, _ctrl_block{new ControlBlock<T>()} { __increment_weakptr(); CHECK }
    
    WeakPtr(const WeakPtr& other) noexcept : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    { 
        __increment_weakptr(); 
        CHECK 
    }

    WeakPtr(const SharedPtr<T>& other) noexcept 
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    { 
        if(_ptr != nullptr && _ctrl_block != nullptr)
            _ctrl_block->inc_wref();
        CHECK 
    }

    ~WeakPtr() { __decrement_weakptr(); CHECK }

    void reset() noexcept
    {
        this->__decrement_weakptr();
    }
    
    bool expired() noexcept
    {
        if (_ctrl_block == nullptr)
            return true;

        if (_ctrl_block->_ref_count == 0)
            --_ctrl_block->_weak_count;

        if (_ctrl_block->_ref_count + _ctrl_block->_weak_count == 0) {
            delete _ctrl_block;
            _ptr = nullptr;
            _ctrl_block = nullptr;
        }

        return !_ctrl_block || _ctrl_block->_weak_count == 0;
    }

    auto lock()
    {
        return expired() ? SharedPtr<T>() : SharedPtr<T>(*this);
    }

    bool Invariant() const noexcept 
    {
        if (_ptr == nullptr)
            return true;
        return _ctrl_block->_weak_count > 0;
    }
    
    template<class T> 
    friend void swap(WeakPtr<T>& lhs, WeakPtr <T>& rhs) noexcept;

    WeakPtr& operator=(const WeakPtr& other) 
    {
        if (this != &other) {
            __decrement_weakptr();
            _ptr = other._ptr;
            _ctrl_block = other._ctrl_block;
            __increment_weakptr();
        }

        CHECK
        return *this; 
    }

    WeakPtr& operator=(const SharedPtr<T>& other) {
        _ptr = other._ptr;
        _ctrl_block = other._ctrl_block;

        if(_ptr != nullptr && _ctrl_block != nullptr)
            _ctrl_block->inc_wref();

        CHECK
        return *this;
    }

private:
    void __increment_weakptr()
    {
        if (_ctrl_block != nullptr) {
            _ctrl_block->inc_wref();
        }
    }

    void __decrement_weakptr()
    {
        if (_ctrl_block != nullptr && _ptr != nullptr) {
            if (--_ctrl_block->_weak_count == 0 && _ctrl_block->_ref_count == 0) {
                delete _ctrl_block;
                delete _ptr;
            }
        }
        _ptr = nullptr;
        _ctrl_block = nullptr;
    }
};

template<class T>
inline void swap(WeakPtr<T>& lhs, WeakPtr<T>& rhs) noexcept
{
    std::swap(lhs, rhs);
}

Answer 1

General

Overall very good.

The double underscore is reserved for the implementation. You should not be using it in your code.

Also the rules for leading underscore are non-trivial enough that most people either don't know the rules or get them wrong. Even though you don't it still not a good idea as others will get things wrong, and it will become an issue.

What are the rules about using an underscore in a C++ identifier?

The trailing underscore is perfectly fine, so a minor modification, and you can get the same pattern to help you identify your member variables (though I don't personally think this is a good idea as it tends to lead to sloppy variable naming that makes cut-and-paste mistakes easier).

Bugs

I believe you have a bug in the shared_ptr constructor that takes a weak_ptr.

    explicit SharedPtr(WeakPtr<T>& other)
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    {
        if (other.expired())
            throw std::bad_weak_ptr();
        

        // Why are you increneting the weak_pointer count?
        // You just created a shared pointer.
        _ctrl_block->inc_wref();
        CHECK
    }

---

This does not make sense.

    WeakPtr(T* other) : _ptr{other}, _ctrl_block{new ControlBlock<T>()} { __increment_weakptr(); CHECK }

You can never get a shared pointer to that object (as there has to exist at least one shared pointer before a weak pointer can be converted to a shared pointer as otherwise the pointed at object will have been deleted.

I suspect that you are also leaking the other pointer as a result.

---

This looks wrong.

Is this not a const method that simply checks the state of what the weak pointer is pointing at?

    bool expired() noexcept
    {
        if (_ctrl_block == nullptr)
            return true;

        // Not sure why you are decrementing the reference count here.
        // You are only checking if the current weak pointer is valid
        // Which simply requries you to check if there still exists a 
        // a shared pointer.
        if (_ctrl_block->_ref_count == 0)
            --_ctrl_block->_weak_count;

        // This is doubly wrong.
        if (_ctrl_block->_ref_count + _ctrl_block->_weak_count == 0) {
            delete _ctrl_block;
            _ptr = nullptr;
            _ctrl_block = nullptr;
        }

        return !_ctrl_block || _ctrl_block->_weak_count == 0;
    }

I would simply write it like this:

    bool expired() const noexcept
    {
        return _ctrl_block == nullptr || _ctrl_block->_ref_count == 0;
    }

---

You have not defined the member values here

    WeakPtr() { CHECK }

Thus they are currently indeterminate. Likely to cause some undefined behaviour if you use an object created like this.

This bug may not even be caught by check. As debug code will generally initialize the memory with all zeros (to help in debuggin) and in production the assert() will be disabled.

Corner Case Issues

You should declare the operator bool() as explicit.

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

This prevents certain corner cases. Check out here

This version of operator bool will convert your shared object to bool in a few other instances. For example:

SharedPtr<int>    value1(new int(8));
SharedPtr<int>    value2(new int(9));

if (value1 == value2) {
    // unfortunately, this will print "They match".
    // Because both values are converted to bool (in this case true).
    // Then the test is done.
    std::cout << "They match\n";
}

To prevent this::

    explicit operator bool() const noexcept { return _ptr != nullptr && _ctrl_block != nullptr; }
 // ^^^^^^^^

Compiler Warnings:

Does this not give you a compiler warning. You have two definitions of CHECK. The compiler should tell you that you are redefining CHECK as you include both header files that include a definition.

#define CHECK assert(Invariant());

Why is this a macro?

#define CHECK assert(Invariant());

I don't think you gain any advantage over this being a macro over an inline function.

That fact that it can be redefined or have another definition may be a good reason that this should not be a macro, but rather an inline function.

Actual Compiler Warnings generated by gcc

> g++ -std=c++20 -Wall -Wextra -Werror -pedantic <file>.cpp
error: declaration of 'T' shadows template parameter
template<class T> friend void swap(SharedPtr<T>& lhs, SharedPtr<T>& rhs) noexcept;

warning: '&&' within '||'
else if (_ptr != nullptr || _ctrl_block != nullptr &&
                         ~~ ~~~~~~~~~~~~~~~~~~~~~~~^~

Code Review

This is very old school:

 template<class T>

This imlies that T can only be a class. So more modern approach is to use typename.

 template<typename T>

---

Not sure why this is a template!

template<class T>
struct ControlBlock
{
    int _ref_count;
    int _weak_count;
    void inc_ref() noexcept { ++_ref_count; }
    void inc_wref() noexcept { ++_weak_count; }
    void dec_ref() noexcept { --_ref_count; }

    // Skeptical over this delete.
    // Though I don't see it being misuesed in the code below
    // A call to `delete **this**` is very much an antipattern.
    // This is likely to cause errors down the road.
    void dec_wref() noexcept { if (--_weak_count == 0) delete this; }

    int use_count() const noexcept { return _ref_count; }
    ControlBlock() : _ref_count{ 0 }, _weak_count{ 0 } {}
    ~ControlBlock() { _ref_count = 0; _weak_count = 0; }

};

There is no reference to T in the class.

---

Why the check on _ptr here.

    void __increment_reference()
    {
        if (_ptr != nullptr && _ctrl_block != nullptr)
                _ctrl_block->inc_ref();
    }

If the _ptr is nullptr then should _ctrl_block not already be nullptr. Seems like a wasted check. And in the next section, __remove_reference() you don't check the _ptr.

I would note that this is one time you don't use '{}` around the conditional code. I would always use the braces (even for simple one liner), and for consistency you should use the same style as everywhere else.

---

    void __remove_reference()
    {
        if (_ctrl_block && _ctrl_block->_ref_count > 0) {
            --_ctrl_block->_ref_count;

            // You have already established that _ctrl_block is not nullptr
            // You don't need to check again.
            if(_ctrl_block && _ctrl_block->_ref_count == 0){
                delete _ptr;
            }

---

Please initialize one variable per line.

    constexpr SharedPtr() : _ptr{ nullptr }, _ctrl_block{ nullptr } { }
    explicit SharedPtr(T* other) : _ptr{ other }, _ctrl_block{new ControlBlock<T>}

Just like in normal code where it is standard to only initialize one variable per line. It is a nice to only initialize one variable per line in the initializer list. Remember this is being read by humans not machines. Be nice to your fellow human.

---

Congrats on getting the nullptr case. Most people miss this.

    constexpr SharedPtr(const std::nullptr_t) noexcept 
        : _ptr{ nullptr }, _ctrl_block{ nullptr } { CHECK }

---

Good use of std::exchange(). I am still getting used to using that.

    SharedPtr(SharedPtr&& other) noexcept 
        : _ptr{ std::exchange(other._ptr, nullptr) }, 
          _ctrl_block{std::exchange(other._ctrl_block, nullptr)} 

---

Normally I am against the assignment to self test, because it is better handled by the copy and swap idiom. But I think in this case we can argue that it is useful.

    SharedPtr& operator=(const SharedPtr& other) noexcept 
    {
        if (this == &other)
            return *this;

BUT. I think a better test is simply to test if they are both using the same control block. If they are then we don't need to change anything, and it covers the assignment to self at the same time.

So I would change the above to:

        if (_ctrl_block == other._ctrl_block)
            return *this;

---

    SharedPtr& operator=(std::nullptr_t)
    {

        // Like `__remove_reference()`, I think the check on
        // `_ptr` is not required.
        if (_ptr == nullptr && _ctrl_block == nullptr)
            return *this;

---

OK. This works. But the point of writing your own swap() is to have a more efficient one than the standard implementation. If you are simply going to use the standard version, then there is no point in writing your own.

template<class T> void swap(SharedPtr<T>& lhs, SharedPtr<T>& rhs) noexcept 
{
    std::swap(lhs, rhs);
}

Normally, I implement the swap function in terms of a swap method.

template<class T>
void swap(SharedPtr<T>& lhs, SharedPtr<T>& rhs) 
{
    lhs.swap(rhs);
}

void SharedPtr::swap(SharedPtr& other) noexcept
{
    using std:swap;
    swap(ptr, other.ptr);
    swap(ctrl, other.ctrl);
}

---

In this constructor, you call __increment_weakptr().

    WeakPtr(const WeakPtr& other) noexcept : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    { 
        __increment_weakptr(); 
        CHECK 
    }

But in the very next one, you call inc_wref() on the control block directly.

    WeakPtr(const SharedPtr<T>& other) noexcept 
        : _ptr{other._ptr}, _ctrl_block{other._ctrl_block}
    { 
        if(_ptr != nullptr && _ctrl_block != nullptr)
            _ctrl_block->inc_wref();
        CHECK 
    }

Self Plug:

Please also have a read of the article I wrote about writting smart pointers:

https://lokiastari.com/series/

Answer 2

There are several conceptual issues with the class. I'll just address what is lacking in the class-design.

1. It is not thread-safe. The control block's main aim is to ensure thread-safety when deleting the object ensuring proper lifetime end instead of data race. Note: you don't need to ensure that the object is properly used - that's up to user, just make sure that the control block is and that call to destructor is proper.

2. Unlike, std::shared_ptr, there is no option to use custom deleters. These can be very important for multiple uses. Starting with ensuring safe deleting of objects from other dlls and ending with completely custom calls.

3. There is no support for interface classes or any polymorphism in general. Even with std::unique_ptr you can have pointer towards an interface. With std::shared_ptr the interface doesn't even need to have a virtual destructor. Here it is simply not supported at all. And this is a very fundamental feature of pointers.

4. Also, std::make_shared allocates the control block and the class within a single memory allocation - saving an unnecessary call to new.