Overview

Personally I would always put my code in its own unique namespace. I don't think SharedPtr is going to be that unique and you don't want to have an accidental clash.

Shared pointers are not easy to get correct.
There is a famous article by Scott Myers about his attempt at shared pointers for one of his books. Where he was getting corrections for several years after the attempt.

Now don't get me wrong. I think everybody should try. Its a good exercise. BUT nobody should use anything but a "standard" (well tested) shared pointer because of the easy holes that you will fall into.

I found four situations where your smart pointer is broken.

• Constructor
• Standard Copy assignment
• Pointer assignment
• The destroy can be used multiple times (double delete issue)

It makes me squirm a bit that a nullptr will have a reference count and when a smart pointer holding a nullptr is copied that reference counter is incremented.

Additionally the code in two constructor and destructor is repeated (testing for zero reference count and deleting the object and counter). This you should simplify (KISS and Clean Code) by factorizing common repeated code into a function that is used from all these places.

I would also like to be able to use move semantics with a smart pointer, so I personally think you should add move semantics to your object (but if you have a simpler use case then maybe you can justify not having it (and you should justify it in some comments)).

You don't write const correct code so you need to pay a bit more attention there.

There is a some missing/broken functionality (seem my articles for more details). But things like nullptr construction and testing for equality are missing.

See my articles on smart pointers for more reading:

UniquePointer
SharedPointer
Smart-Pointer Constructors

Code Review

I don't like this.

#pragma once

It's not portable. And it's so simple to do it the standard way!

The use of #define is exceedingly rare in C++. In C it is needed for a lot of situations but most of those situations have specific C++ idioms that are type safe and idiomatic.

#define usage_count_type uint16_t

The C++ way of doing this:

using usage_count_type = uint16_t;

OK: You have a ref counter.

class RefCounter {
public:
    usage_count_type count = 0;
    bool destroyed = false;

    RefCounter() {}

    RefCounter(const RefCounter&) = delete;
    RefCounter& operator=(const RefCounter&) = delete;
};

Potentially allowing the user to delete the object before 0 is reached and tracking that. The standard allows this via std::weak_ptr with slightly better semantics than you propose.

Missing optimizations where the value and counter can be allocated at the same time for efficiency (if you want).

But OK as a starting point.

First issue is that you are not using the initializer list.

    explicit SharedPtr(T* ptr = nullptr) noexcept {
        pointer = ptr;
        counter = new RefCounter();
        counter->count++;
    }

OK. It does not matter in this version since they are pointers so you are not losing anything NOW. But one of the major way things get updated in C++ is to simply change the type of a member. This may result in your code becoming not optimal if you are not using the initializer list. So get into the habit of using the initializer list to make sure you always generate optimal code.

    explicit SharedPtr(T* ptr = nullptr) noexcept
        : pointer{ptr}
        , counter{new RefCounter()}
    {
        counter->count++;
    }

OK. Your first real issue.

    explicit SharedPtr(T* ptr = nullptr) noexcept {
        pointer = ptr;
        counter = new RefCounter();
        counter->count++;
    }

The use of noexcept is wrong here. You are trying to tell the compiler this function can't have an exception. But it can: new has the potential of throwing. If it does throw, then an exception here ends the program without unwinding the stack and therefore without calling any destructors. This is a bad idea.

So we should correct to remove the noexcept.

What happens if the new fails? You have now leaked the ptr that passed in as a parameter.

The point of this constructor is you pass a pointer (intending for the object to take ownership and manage the pointer) and no matter what happens you must make sure that this pointer is not leaked. That means if an an exception happens during the constructor you must release the pointer (because if an exception escapes the constructor then the destructor will never be called).

    explicit SharedPtr(T* ptr = nullptr)
    try
        : pointer{ptr}
        , counter{new RefCounter()}
    {
        counter->count++;
    }
    catch(...)
    {
        delete ptr;
        throw;
    }

OK you have a copy constructor.

    // Copy constructor
    SharedPtr(SharedPtr<T>& sp) {
        pointer = sp.pointer;
        counter = sp.counter;
        counter->count++;
    }

Looks very standard. But again I would use the initializer list.

    // Copy constructor
    SharedPtr(SharedPtr<T>& sp)
        : pointer{sp.pointer}
        , counter{sp.pointer}
    {
        counter->count++;
    }

OK second mistake is in the copy assignment operator:

2: No check for self assignment.

   int main()
   {
       Shared_ptr<int>  a(new int(12));
       a = a;
          // Decrements the counter.
          // Destroys the object.
          // Destroys the "counter"
          // Makes it point at its orignal value.
          // increments the **DELETED** counter object!!!

          // a is now completely invalid.
   }

Let's walk through the assignment operator:

    SharedPtr<T>& operator=(SharedPtr<T>& sp) {
        // Note: this and &sp are the same.

        // This is the only reference to the counter.
        counter->count--;
        // So decrementing it will cause it to reach zero.
        if (counter->count == 0) {
            // It has not been destroyed before
            // So destroy the object.
            if (!counter->destroyed) delete pointer;

            // destroy the counter.
            delete counter;
        }

        // This just assings the same values back.
        // Both point at deleted objects.
        // **BUT** destroyed is still "false" in the released
        // memory so even if nothing bad happens soon
        // you are going to eventually get a double call to delete
        // on this memory in the futre.
        pointer = sp.pointer;
        counter = sp.counter;

        counter->count++;

        return *this;
    }

A lot of beginners will simply check for assignment to self and return immediately if this happens. But this is a rare situation and by using this technique you are pessimizing the normal operation (the normal situation is not self assignment). So the better technique is to use the copy and swap idiom.

     // Notice the parameter is passed by value.
     // This effectively makes a copy of the object
     // what is required in normal situations.
     SharedPtr<T>& operator=(SharedPtr<T> sp)
     {
         std::swap(pointer, sp.pointer);  // You are copying the content
         std::swap(counter, sp.counter);  // out of the newly created object.
         return *this;
     }
     // At this point the parameter `sp` is destroyed.
     // This will call release on any pointer via the destructor
     // if this was the only reference to the object.

Your assignment with a RAW pointer is also broken:

// Overload = operator for pointer of type T
SharedPtr<T>& operator=(T* object) {

    // This part is correct.
    counter->count--;
    if (counter->count == 0) {
        if (!counter->destroyed) delete pointer;
    }

    pointer = object;

    // But you can't re-use this counter if the count is not zero.
    // This counter object may already be being used by other
    // objects.
    counter->count = 1;

    return *this;
}

3: You increment the counter that could potentially point at another object.

Think about this:

 int main()
 {
     SharedPtr<int>  a(new int(14));
     SharedPtr<int>  b(a);
     SharedPtr<int>  c(a);

     c = new int(15);
     // Note: This decrements the counter.
     //       But then re-uses the counter object.
     //       Which is the same object that is being used
     //       by both a and b but now the count is 1
     //       even though all three variables are using the same counter
     //       and two of them have different pointers associated!
}
// When these variables go out of scope
// only c will be deleted.
// variables b and a will have a negative count (or very large)
// and thus the associated memory is leaked.

Again the solution is to use Copy and Swap idiom:

SharedPtr<T>& operator=(T* object) {
     // This effectively makes a new smart pointer
     SharedPtr<T> sp(object);

     std::swap(pointer, sp.pointer);  // You are copying the content
     std::swap(counter, sp.counter);  // out of the newly created object.
     return *this;
}
// At this point the local variable `sp` is destroyed.
// This will release via the destructor any value that was being pointed at.

You don't check if the pointer has already been destroyed!

void destroy() {
    delete pointer;
    counter->destroyed = true;
}

This could result in a double delete if multiple different pointers try and release the memory.

void destroy() {
    // Make sure you check it is not already deleted.
    if (!counter->destroyed) {
        delete pointer;
        counter->destroyed = true;
    }
}

Mark these functions as const:

    uint16_t use_count() {
        return counter->count;
    }



    bool is_destroyed() {
        return counter->destroyed;
    }

It does not change the state so let the compiler know it is safe to use in const context.

Sure this works:

    // Get the pointer
    T* get() {
        return pointer;
    }

    // Overload * operator
    T& operator*() {
        return *pointer;
    }

    // Overload -> operator
    T* operator->() {
        return pointer;
    }

But what if your shared pointer is const? You may still want to use what is in the object (reading any values). So you should also have const versions of these functions:

Retry:

I am now going to make the same mistake as Scott Myers and show what I would do.

#ifndef THORSANVIL_SMARTPTR_H
#define THORSANVIL_SMARTPTR_H

#include <cstdint>

namespace ThorsAnvil::SmartPtr
{

using UsageCount = uint16_t;


template <typename T>
class SharedPtr {
    class RefCounter {
    public:
        UsageCount count = 0;
        bool destroyed = false;
    
        RefCounter() {}
    
        RefCounter(const RefCounter&) = delete;
        RefCounter& operator=(const RefCounter&) = delete;
    };

    T*          pointer;
    RefCounter* counter;

public:
    SharedPtr(std::nullptr_t)
        : pointer( nullptr )
        , counter( nullptr )
    {}

    explicit SharedPtr(T* ptr = nullptr)
    try
    {
            : pointer{ ptr }
            , counter{ ptr ? new RefCounter() : nullptr}
        {
            increment();
        }
    }
    catch(...)
    {
        delete ptr;
        throw;
    }

    ~SharedPtr()
    {
        decrement();
    }

    // Copy constructor
    SharedPtr(SharedPtr& sp) noexcept
        : pointer{ sp.pointer }
        , counter{ sp.counter }
    {
        increment();
    }

    // Move constructor
    SharedPtr(SharedPtr& sp) noexcept
        : pointer{ std::exchange(sp.pointer, nullptr) }
        , counter{ std::exchange(sp.counter, nullptr) }
    {}

    // Overload = operator
    // Both Copy and Move in a single function.
    SharedPtr& operator=(SharedPtr sp) noexcept
    {
        swap(sp);
        return *this;
    }

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

    friend void swap(SharedPtr& lhs, SharedPtr& rhs)
    {
        lhs.swap(rhs);
    }

    // Overload = operator for pointer of type T
    SharedPtr& operator=(T* object)
    {
        SharedPtr sp(object);
        swap(sp);
        return *this;
    }

    uint16_t use_count() const {
        return counter ? counter->count : 0;
    }

    bool is_destroyed() const {
        return counter ? counter->destroyed : true;
    }

    void destroy() {
        if (!is_destroyed()) {
            delete pointer;
            counter->destroyed = true;
        }
    }

    explicit operator bool() const
    {
        return !is_destroyed();
    }

    // Get the pointer
    T*       get()              {return pointer;}
    T const* get() const        {return pointer;}

    // Overload * operator
    T&       operator*()        {return *pointer;}
    T const& operator*() const  {return *pointer;}

    // Overload -> operator
    T*       operator->()       {return pointer;}
    T const* operator->() const {return pointer;}

private:
        void increment()
        {
            if (counter) {
                ++counter->count;
            }
        }
        void decrement()
        {
            if (counter) {
                --counter->count;
                if (counter->count == 0)
                    if (!counter->destroyed) {
                        delete ptr;
                    }
                    delete counter;
                }
            }
        }

};

}
#endif

Use std::weak_ptr

So, I decided to use std::shared_ptr. But the standard shared pointer is not able to delete the managed pointer before the usage count reaches zero.

Indeed, that is the case if you just use std::shared_ptr. However, you can use std::weak_ptr to get a handle to a std::shared_ptr that doesn't affect the reference count.

#include <iostream>
#include <memory>

struct Entity {
    ~Entity() {
        std::cout << "Destructing entity\n";
    }
};

int main() {
    auto e1 = std::make_shared<Entity>();
    auto e2 = std::weak_ptr(e1);
    std::cout << "Use count before: " << e2.use_count() << '\n';
    e1.reset();
    std::cout << "Use count after: " << e2.use_count() << '\n';

    if (auto e3 = e2.lock()) {
        std::cout << "Object still alive\n";
    } else {
        std::cout << "Object deleted\n";
    }
}

And all this is thread-safe.

Just curious what is the use case? If game engine is already using an std::shared_ptr then it will be destroyed only when the count is 0. If it does not do that then sticking a pointer to game objects in an std::shared_ptr is dangerous. Once the pointer goes out of scope it will call delete on the memory location and if the game engine used anything other than new to allocate the object, you just invoked undefined behavior. And even if the Game engine uses new, you just created a potential use after free. If you are creating an object via shared_ptr and giving it to the game engine to manage then you can have similar undefined behaviors too.

I will assume you have no use case for a custom deleter so it is not implemented.

1. Dont use #pragma once different platforms implement it differently. Use #define header guards instead.
2. Use using usage_count_type = uint16_t not a macro.
3. You dont have delete copy assignment and construction for RefCounter. It does not need a constructor either.
4. SharedPtr constructor is not noexcept it makes an allocation in it body which means it can throw
5. Use copy-swap idiom to implement copy and move constructors and assignment operators.
6. Copy constructors take a const reference not a non-const reference
7. Implement move assignment. Look up rule of 5.
8. Implement const-qualified versions of dereference operators and get()
9. Implement operator bool and comparision operators
10. use_count() must const and noexcept
11. Combine nested if like this if(counter->count == 0 && !counter->destroyed)
12. Counter must be thread safe.