Intrusive smart pointer implementation

Question

I have homework in college to implement intrusive smart pointers.

#include <atomic>
#include <string_view>
#include <string>
#include <iostream>

struct RefCount
{
    inline void IncrementRefCount() const noexcept
    {
        refCount.fetch_add(1, std::memory_order_relaxed);
    }

    inline void DecrementRefCount() const noexcept
    {
        refCount.fetch_sub(1, std::memory_order_acq_rel);
    }

    [[nodiscard]] inline uint32_t getRefCount() const noexcept { return refCount; }

private:
    mutable std::atomic_uint32_t refCount = 0;
};

template<typename T>
struct Ref 
{
    Ref() = default;

    Ref(std::nullptr_t) noexcept
        : data(nullptr)
    {
    }

    Ref(T* instance) noexcept
        : data(instance)
    {
        IncrementRef();
    }

    template<typename T2>
    Ref(const Ref<T2>& other) noexcept
    {
        data = (T*)other.data;
        IncrementRef();
    }

    template<typename T2>
    Ref(Ref<T2>&& other) noexcept
    {
        data = (T*)other.data;
        other.data = nullptr;
    }

    ~Ref() noexcept
    {
        DecrementRef();
    }

    Ref(const Ref<T>& other) noexcept
        : data(other.data)
    {
        IncrementRef();
    }

    [[nodiscard]] inline Ref& operator=(std::nullptr_t) noexcept
    {
        DecrementRef();
        data = nullptr;
        return *this;
    }

    [[nodiscard]] inline Ref& operator=(const Ref<T>& other) noexcept
    {
        other.IncrementRef();
        DecrementRef();

        data = other.data;
        return *this;
    }

    template<typename T2>
    [[nodiscard]] inline Ref& operator=(const Ref<T2>& other) noexcept
    {
        other.IncrementRef();
        DecrementRef();

        data = other.data;
        return *this;
    }

    template<typename T2>
    [[nodiscard]] inline Ref& operator=(Ref<T2>&& other) noexcept
    {
        DecrementRef();

        data = other.data;
        other.data = nullptr;
        return *this;
    }

    [[nodiscard]] inline operator bool() noexcept { return data != nullptr; }
    [[nodiscard]] inline operator bool() const noexcept { return data != nullptr; }

    [[nodiscard]] inline bool operator==(const Ref<T>& other) const noexcept
    {
        return data == other.data;
    }

    [[nodiscard]] inline bool operator!=(const Ref<T>& other) const noexcept
    {
        return !(*this == other);
    }

    [[nodiscard]] inline T* operator->() noexcept { return data; }
    [[nodiscard]] inline const T* operator->() const noexcept { return data; }

    [[nodiscard]] inline T& operator*() noexcept { return *data; }
    [[nodiscard]] inline const T& operator*() const noexcept { return *data; }

    [[nodiscard]] inline T* get() noexcept { return data; }
    [[nodiscard]] inline const T* get() const noexcept { return data; }

    inline void Reset(T* instance = nullptr) noexcept
    {
        DecrementRef();
        data = instance;
    }

    template<typename T2>
    [[nodiscard]] inline Ref<T2> as() const noexcept
    {
        return Ref<T2>(*this);
    }

    template<typename... Args>
    [[nodiscard]] inline static Ref<T> Create(Args&&... args) noexcept
    {
        return Ref<T>(new T(std::forward<Args>(args)...));
    }

private:
    inline void IncrementRef() const noexcept
    {
        if (data) {
            data->IncrementRefCount();
        }
    }

    inline void DecrementRef() const noexcept
    {
        if (data) {
            data->DecrementRefCount();
            if (data->getRefCount() == 0) {
                delete data;
            }
        }
    }

    template<typename T2>
    friend struct Ref;

    T* data = nullptr;
};

template<typename T>
class WeakRef
{
public:
    WeakRef() noexcept = default;

    WeakRef(Ref<T> ref) noexcept
    {
        data = ref.get();
    }

    WeakRef(T* instance) noexcept
        : data(instance)
    {
    }

    [[nodiscard]] inline Ref<T> lock() const noexcept { return Ref<T>(data); }

    [[nodiscard]] inline bool isValid() const noexcept { return data; }
    [[nodiscard]] inline operator bool() const noexcept { return isValid(); }

private:
    T* data = nullptr;
};

struct Leaf;
struct Node : RefCount
{
    virtual ~Node() noexcept = default;

    void PrintLeafName() noexcept const;

    Ref<Leaf> leaf = nullptr;
    std::string name;

protected:
    Node(const std::string_view name) noexcept
        : name(name)
    {
    }
};

struct ConcreteNode : Node
{
    ConcreteNode(const std::string_view name, const std::string_view leafName) noexcept;
    ~ConcreteNode() noexcept override = default;
};

struct Leaf : RefCount
{
    virtual ~Leaf() noexcept = default;

    virtual void PrintName() const noexcept = 0;
    void PrintNodeName() const noexcept
    {
        std::cout << parent.lock()->name << "\n";
    }

protected:
    Leaf(WeakRef<Node> parent, const std::string_view name) noexcept 
        : parent(std::move(parent)), name(name)
    {
    }

    WeakRef<Node> parent;
    std::string name;
};

struct ConcreteLeaf : Leaf
{
    ConcreteLeaf(WeakRef<Node> parent, const std::string_view name) noexcept 
        : Leaf(std::move(parent), name)
    {
    }
    ~ConcreteLeaf() noexcept override = default;

    void PrintName() const noexcept
    {
        std::cout << name <<"\n";
    }
};

ConcreteNode::ConcreteNode(std::string_view name, std::string_view leafName) noexcept
    : Node(name)
{
    leaf = Ref<ConcreteLeaf>::Create(this, leafName);
}

void Node::PrintLeafName() const noexcept
{
    leaf->PrintName();
}

int main()
{
    Ref<Node> node = Ref<ConcreteNode>::Create("Concrete node", "Concrete leaf");

    node->PrintLeafName();
    node->leaf->PrintNodeName();

    return 0;
}

Could you review it please? I'm not sure if RefCount class needs a virtual destructor. Ref class deletes T, so I don't think it's needed, but I may be wrong.

I'm also not sure of mutable std::atomic_uint32_t refCount, but it's required for RefCount::as() const

https://godbolt.org/z/cvGzaqvqe

Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. — Mast
– Mast ♦, Commented Dec 29, 2022 at 11:22

ALX23z · Accepted Answer · 2022-12-29 14:48:18Z

Bugs: WeakRef is a big fat bug. Say you have your smart pointer Ref, create one WeakRef from it. Then clear Ref. Then use lock() function on WeakRef... voila a UB as you address deleted memory.

std::shared_ptr and std::weak_ptr implement the whole thing by keeping control block separately. Once all shared_ptr are gone the object is deleted. And only once all weak_ptr are gone too, the control block is deleted too. Not sure how you should implement it but the managed object needs to be destroyed once all references are gone while the mechanism behind weak references needs to be kept alive as long as some weak references still exist. Do you really need weak references with intrusive smart pointers? For me it feels that the two don't mesh well together.

The acquire_release memory order in DecrementRefCount() is overkill. You need only to call release regularly. While apply acquire fence only prior to destroying the object. Also the routine, DecrementRef() in Ref is buggy. You first decrease the value then read it. What if it is decreased simultaneously in two places and then it is read simultaneously in two places? You'll get double-delete. DecrementRefCount() should return the value given by fetch_sub and use it to judge whether to delete or not.

Assignment operators: make sure operation like self-assignment (x = x; and x = std::move(x);) are safe. Currently they might behave weirdly or do unnecessary operations. While it doesn't normally happen people might accidentally trigger it in complicated calls.

Operators like ->, *, and get() ought to be const and return the object without unnecessary const. Here constness means that the pointer is unchanged but the data is under users' responsibility not the smart pointer's. It is probably overkill for your homework assignment but the class should support T being a const object. std::shared_ptr<const double> is a thing.

Clarification: Say you have a function foo(const Ref<A>& a). And bar() is not a const function of A. Then writing a->bar() is a compilation error because -> returns a const pointer. However, one can write Ref<A> a2 = a; and then call a2->bar(); which is absurd, right? So returning a const pointer via -> is illogical. The situation with * and get() is the same.

RefCount's operations should be only accessible to Ref and WeakRef.

Writing const std::string_view in function calls is superfluous. Write simply std::string_view.

The class is inconvenient to use. Ref<int> and Ref<std::string> will not compile. It should be able to figure out on its own if the object does not have the control block mechanism and add it automatically while wrapping and unwrapping all the calls for the user's convenience.

The class does not work with polymorphism. It is frequent that one first declares an Interface and then creates a Derived class. Normally users want to hold smart pointer to Interface, right? With this implementation it is impossible.

You shouldn't put noexcept everywhere. There are places where it is much more reasonable to not have it. They are only important for move constructor and move assignment else STL algorithm won't work right with the classes.

Regarding your questions:

Self-Assignment:

Ref<A> x;
x = x;
x = std::move(x);

Well, nobody would ever write that, right? But imagine situation:

bool DoStuff(Ref<A>& input, Ref<A>& output)
{
     // lots of convoluted and branched code, some with early returns.
     output = std::move(input);
     return true;
}

And then someone decided to use it with DoStuff(x,x) for whatever reason. That'll cause a bug if self-assignment were not safe. That's being said, some STL classes do weird stuff on self-move-assignment, like std::vector that clears itself.

The class does not work with polymorphism: Okey I didn't check it fully and just saw that you didn't implement any proper mechanisms.

First, your

template<typename T2>
Ref(const Ref<T2>& other) noexcept
{
    data = (T*)other.data;
    IncrementRef();
}

Is extremely unsafe. With explicit pointer cast you can cast completely unrelated classes. For implicit conversion you ought to require T is a base of T2. When it is not the case, you ought to write external functions that perform dynamic casting and static casting. Like std::dynamic_pointer_cast and std::static_pointer_cast do for std::shared_ptr. Otherwise, you can easily cast cats into bananas.

Also, as it is now, it is also buggy. Calling delete on pointer to an interface will cause UB. That's because the destructor is not virtual. RefCounter should declare its destructor virtual.

If std::make_shared() and the like are used, the control-block is not separate in better implementations. Destroying the object (without freeing it, as coalesced allocation) and destroying the control-block (including freeing it, and any coalesced allocation) are still cleanly separated though. — Deduplicator
– Deduplicator, Commented Dec 29, 2022 at 3:37
@Deduplicator it is not separate per C++ standart. But it doesn't follow intrusive smart pointer logic and requires each smart pointer instance to have two pointers, to the data and to the control block. — ALX23z
– ALX23z, Commented Dec 29, 2022 at 7:21
OP has asked you some questions in their answer here. The answer will eventually be taken down, but feel free to discuss further here in the comments. — Mast
– Mast ♦, Commented Dec 29, 2022 at 11:25
@Edziju yeah, only that return *this; instead of plain return;. For shared pointers you can compare pointers to avoid unnecessary increment+decrement when the pointers manage the same object, not essential by any means. — ALX23z
– ALX23z, Commented Dec 29, 2022 at 14:45
What if WeakRef::Lock() would check if T* is inside a static std::unordered_set<void*> validReferences and if it's not it will return a nullptr? Is it a good idea? — Edziju
– Edziju, Commented Dec 30, 2022 at 13:06

Stack Exchange Network

Intrusive smart pointer implementation

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Intrusive smart pointer implementation

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions