7
\$\begingroup\$

I wrote a simple lighter version of std::shared_ptr<T> called res_ptr<T>.

std::shared_ptr<T> is somewhat heavy due to all the nuances of its control block: support for safe deleting without virtual destructor, support for std::weak_ptr<T> (I am not certain on all details of the implementation... but I believe that it either requires an extra allocation for the control block, or has complicated logic on when to delete...).

The res_ptr<T> requires T to inherit from class resource that manages the reference counter and deletes the instance upon destruction once the counter reaches zero requires. Yes, it doesn't support array version res_ptr<T[]> and I don't think that it should but I might wrong.

res_version<T> is a helper class that attaches resource class to classes that do not inherit from resource that shouldn't change anything besides that.

Besides that, it is supposed to have fairly same interface as std::shared_ptr<T> and std::unique_ptr<T>. I know it currently lacks option for custom deleter but I don't think that I'll ever need it so I don't intend to implement it.

The implementation I have is written on Visual Studio and isn't fully portable. Though, honestly, this class is going to be the least of my worries once I need portability for the codebase. Though, if it is something else besides modifying #pragma once or issues with initialization of std::atomic I'd like to know.

I'd like to know if there are any issues or downsides that I failed to consider as well as general code review.

#pragma once
#include <atomic>

class resource
{
public:
    virtual ~resource()         = default;

    resource()                  = default;

    // moving / copying does not alter the reference counter
    resource(resource&&)                    noexcept : resource() {};
    resource(const resource&)               noexcept : resource() {};
    resource& operator = (resource&&)       noexcept {};
    resource& operator = (const resource&)  noexcept {};

    void    add_ref()               const   noexcept
    {
        m_refcount.fetch_add(1, std::memory_order_relaxed);
    }

    int     reduce_ref()            const   noexcept
    {
        return m_refcount.fetch_sub(1, std::memory_order_relaxed)-1;
    }

    int     count()                 const   noexcept
    {
        return m_refcount.load(std::memory_order_relaxed);
    }

private:
    mutable std::atomic<int>    m_refcount = 0;
};

template<typename T>
class res_version :
    public T, public resource
{
public:
    template<typename... Args>
    res_version(Args... args):
        T(std::forward<Args>(args)...)
    {};
};

template<typename PResource, typename Enable = void>
class res_ptr;

template<typename PResource>
class res_ptr<PResource, typename std::enable_if_t<std::is_base_of_v<resource, PResource>>>
{
public:
    template<typename PResourceDerived, typename E>
    friend class res_ptr;

    constexpr   res_ptr()           noexcept = default;
    constexpr   res_ptr(nullptr_t)  noexcept {};

    template<typename PResourceDerived, std::enable_if_t<std::is_base_of_v<PResource, PResourceDerived>, int> = 0>
    explicit    res_ptr(PResourceDerived* ptr) : m_ptr(static_cast<PResource*>(ptr))
    {
        if(m_ptr) m_ptr->add_ref();
    };

    ~res_ptr()
    {
        release();
    }

    // copy ctor
    res_ptr(const   res_ptr& ptr) noexcept :
        m_ptr(ptr.get())
    {
        if (m_ptr) m_ptr->add_ref();
    };

    // copy ctor cast
    template<typename PResourceDerived, std::enable_if_t<std::is_base_of_v<PResource, PResourceDerived> && !std::is_same_v<PResource, PResourceDerived>,int> = 0>
    res_ptr(            const   res_ptr<PResourceDerived> & ptr) noexcept :
        m_ptr(static_cast<PResource*>(ptr.get()))
    {
        if (m_ptr) m_ptr->add_ref();
    };

    // move ctor
    res_ptr(                    res_ptr&& ptr) noexcept :
        m_ptr(std::exchange(ptr.m_ptr, nullptr))
    {};

    // move ctor cast
    template<typename PResourceDerived, std::enable_if_t<std::is_base_of_v<PResource, PResourceDerived> && !std::is_same_v<PResource, PResourceDerived>,int> = 0>
    res_ptr(                    res_ptr<PResourceDerived> &&    ptr) noexcept :
        m_ptr(static_cast<PResource*>(std::exchange(ptr.m_ptr, nullptr)))
    {};

    // copy
    res_ptr& operator = (const  res_ptr&        other) noexcept
    {
        if (this != &other)
        {
            release();
            m_ptr = other.m_ptr;

            if (m_ptr) m_ptr->add_ref();
        }

        return *this;
    }

    // move
    res_ptr& operator = (       res_ptr&&       other) noexcept
    {
        if (this != &other)
        {
            release();
            m_ptr = std::exchange(other.m_ptr,nullptr);
        }

        return *this;
    }

    // copy cast
    template<typename PResourceDerived, std::enable_if_t<std::is_base_of_v<PResource, PResourceDerived> && !std::is_same_v<PResource, PResourceDerived>, int> = 0>
    res_ptr& operator = (const  res_ptr<PResourceDerived>&      other) noexcept
    {
        release();
        m_ptr = static_cast<PResource*>(other.m_ptr);

        if (m_ptr) m_ptr->add_ref();

        return *this;
    }

    // move cast
    template<typename PResourceDerived, std::enable_if_t<std::is_base_of_v<PResource, PResourceDerived> && !std::is_same_v<PResource, PResourceDerived>, int> = 0>
    res_ptr& operator = (       res_ptr<PResourceDerived>&& other) noexcept
    {
        release();
        m_ptr = static_cast<PResource*>(std::exchange(other.m_ptr,nullptr));

        return *this;
    }

    PResource* operator ->  ()  const noexcept
    {
        return m_ptr;
    }
    PResource& operator *   ()  const noexcept
    {
        return *m_ptr;
    }
    PResource* get()            const noexcept
    {
        return m_ptr;
    }
    operator bool   ()          const noexcept
    {
        return m_ptr != nullptr;
    }

    void release()
    {
        if (m_ptr && (m_ptr->reduce_ref() == 0))
        {
            delete m_ptr;
        }
    }

    template<typename PResource>
    bool operator == (const res_ptr<PResource>& other) noexcept
    {
        return m_ptr == other.m_ptr;
    }

    template<typename PResource>
    bool operator != (const res_ptr<PResource>& other) noexcept
    {
        return m_ptr != other.m_ptr;
    }

private:
    PResource* m_ptr = nullptr;
};

template<typename PResource, typename... Args>
res_ptr<PResource>          make_resource(Args&& ... args)
{
    return res_ptr<PResource>(new PResource(std::forward<Args>(args)...));
}

template<typename PResourceDerived, typename PResourceBase>
res_ptr<PResourceDerived>   resource_dynamic_cast(const res_ptr<PResourceBase>& uPtr) noexcept
{
    PResourceDerived* ptr = dynamic_cast<PResourceDerived*>(uPtr.get());

    return res_ptr<PResourceDerived>(ptr);
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Do you have any tests? It's worth including them in the review (for two reasons: it helps reviewers run and exercise the code themselves, and it may identify corner cases that need testing). \$\endgroup\$ – Toby Speight Oct 31 at 15:34
  • 1
    \$\begingroup\$ There is a reason that std::shared_ptr is not light wait. Its difficult to write correctly and needs all that code. \$\endgroup\$ – Martin York Oct 31 at 16:28
  • 1
    \$\begingroup\$ @MartinYork res_ptr<T> is not designed to replace std::shared_ptr<T> but to provide a more efficient version for certain classes. \$\endgroup\$ – ALX23z Oct 31 at 16:31
  • \$\begingroup\$ @TobySpeight I don't have any tests dedicated to res_ptr<T>... only tests that utilize res_ptr<T> within larger projects. If you can supplement some good ideas on those edge cases that are worth testing you can post it as an answer. \$\endgroup\$ – ALX23z Oct 31 at 16:42
  • 2
    \$\begingroup\$ @ALX23z not sure I am convinced this is any more efficient than std::shared_ptr (having read the code) you would need to show me evidence of that. Also since std::shared_ptr has much more testing on it I am inclined to think there is something non obvious here that will cause problems (without lots of though going into testing and validating). They found bugs in shared_ptr for well over a decade after it was first written before it got to its current state. It is a good try (probably one of the best I have seen) but this should not be used in production code. \$\endgroup\$ – Martin York Oct 31 at 17:05
5
\$\begingroup\$

Missing headers

  • <cstddef> for std::nullptr_t
  • <type_traits> for std::is_base_of_v, std::is_same_v, std::enable_if_t
  • <utility> for std::exchange, std::forward

Layout

The code is hard to read, with long lines and huge blocks of spaces (perhaps a misguided attempt to align keywords?). Stick to a conventional layout and it will be much easier to read.

Fix the errors and warnings

These should need no further explanation (except perhaps the one caused by misspelling std::nullptr_t):

231599.cpp: In member function ‘resource& resource::operator=(resource&&)’:
231599.cpp:13:55: warning: no return statement in function returning non-void [-Wreturn-type]
   13 |     resource& operator = (resource&&)       noexcept {};
      |                                                       ^
      |                                                       return *this;
231599.cpp: In member function ‘resource& resource::operator=(const resource&)’:
231599.cpp:14:55: warning: no return statement in function returning non-void [-Wreturn-type]
   14 |     resource& operator = (const resource&)  noexcept {};
      |                                                       ^
      |                                                       return *this;
231599.cpp: At global scope:
231599.cpp:43:7: warning: extra ‘;’ [-Wpedantic]
   43 |     {};
      |       ^
      |       -
231599.cpp:57:5: error: non-static data member ‘nullptr_t’ declared ‘constexpr’
   57 |     constexpr   res_ptr(nullptr_t)  noexcept {};
      |     ^~~~~~~~~
231599.cpp:57:34: error: expected ‘;’ at end of member declaration
   57 |     constexpr   res_ptr(nullptr_t)  noexcept {};
      |                                  ^
      |                                   ;
231599.cpp:57:37: error: expected unqualified-id before ‘noexcept’
   57 |     constexpr   res_ptr(nullptr_t)  noexcept {};
      |                                     ^~~~~~~~
231599.cpp:63:6: warning: extra ‘;’ [-Wpedantic]
   63 |     };
      |      ^
      |      -
231599.cpp:83:6: warning: extra ‘;’ [-Wpedantic]
   83 |     };
      |      ^
      |      -
231599.cpp:94:7: warning: extra ‘;’ [-Wpedantic]
   94 |     {};
      |       ^
      |       -
231599.cpp:169:14: error: declaration of template parameter ‘PResource’ shadows template parameter
  169 |     template<typename PResource>
      |              ^~~~~~~~
231599.cpp:49:10: note: template parameter ‘PResource’ declared here
   49 | template<typename PResource>
      |          ^~~~~~~~
231599.cpp:175:14: error: declaration of template parameter ‘PResource’ shadows template parameter
  175 |     template<typename PResource>
      |              ^~~~~~~~
231599.cpp:49:10: note: template parameter ‘PResource’ declared here
   49 | template<typename PResource>
      |          ^~~~~~~~
231599.cpp: In constructor ‘res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>::res_ptr(res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>&&)’:
231599.cpp:87:20: error: ‘exchange’ is not a member of ‘std’
   87 |         m_ptr(std::exchange(ptr.m_ptr, nullptr))
      |                    ^~~~~~~~
231599.cpp: In constructor ‘res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>::res_ptr(res_ptr<PResourceDerived>&&)’:
231599.cpp:93:44: error: ‘exchange’ is not a member of ‘std’
   93 |         m_ptr(static_cast<PResource*>(std::exchange(ptr.m_ptr, nullptr)))
      |                                            ^~~~~~~~
231599.cpp: In member function ‘res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>& res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>::operator=(res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>&&)’:
231599.cpp:116:26: error: ‘exchange’ is not a member of ‘std’
  116 |             m_ptr = std::exchange(other.m_ptr,nullptr);
      |                          ^~~~~~~~
231599.cpp: In member function ‘res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>& res_ptr<PResource, typename std::enable_if<is_base_of_v<resource, PResource>, void>::type>::operator=(res_ptr<PResourceDerived>&&)’:
231599.cpp:139:46: error: ‘exchange’ is not a member of ‘std’
  139 |         m_ptr = static_cast<PResource*>(std::exchange(other.m_ptr,nullptr));
      |                                              ^~~~~~~~
231599.cpp: In function ‘int main()’:
231599.cpp:207:34: error: invalid use of incomplete type ‘class res_ptr<int, void>’
  207 |     auto a = make_resource<int>(5);
      |                                  ^
231599.cpp:47:7: note: declaration of ‘class res_ptr<int, void>’
   47 | class res_ptr;
      |       ^~~~~~~
231599.cpp: In instantiation of ‘res_ptr<PResource> make_resource(Args&& ...) [with PResource = int; Args = {int}]’:
231599.cpp:207:34:   required from here
231599.cpp:186:29: error: return type ‘class res_ptr<int, void>’ is incomplete
  186 | res_ptr<PResource>          make_resource(Args&& ... args)
      |                             ^~~~~~~~~~~~~
231599.cpp:188:12: error: invalid use of incomplete type ‘class res_ptr<int, void>’
  188 |     return res_ptr<PResource>(new PResource(std::forward<Args>(args)...));
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
231599.cpp:47:7: note: declaration of ‘class res_ptr<int, void>’
   47 | class res_ptr;
      |       ^~~~~~~

Usage guidance

My first simple test, expecting a similar interface to std::make_shared(), failed:

auto a = make_resource<int>(5);

So I tried the next most obvious course:

auto a = make_resource<res_version<int>>(5);

This also fails, due to attempting to inherit from a non-struct type.

It seems I need to create a struct (with a constructor) even for something this simple! Much less friendly than the standard shared pointer.

Questionable choices

Why use int to count references? An unsigned type would be more appropriate.

We should check for overflow before incrementing the counter, and throw an exception if that would happen, otherwise we could delete objects still in use. I don't think that's possible without changing away from std::memory_order_relaxed.

The res_version adapter should virtually inherit resource, to avoid the diamond problem when subclasses inherit from more than one of these.

I'd prefer the res_version constructor to include an initializer for the resource base, rather than omitting it. The T() constructor is likely to be tricky here, as it makes it hard for users to select the T{} constructor - important for classes such as std::vector.

The statement if (m_ptr) m_ptr->add_ref(); is used many times - worth encapsulating in a member function (any decent compiler will inline it).

std::is_base_of_v<PResource, PResourceDerived> && !std::is_same_v<PResource, PResourceDerived> is used many times - encapsulate that too (is_derived_from<>, perhaps?).

We haven't implemented swap(), so std::swap() will fall back to copying via a temporary, which implies needless updates to the count.

res_ptr::operator=(res_ptr&&) can be implemented in terms of swap() (if we write that), instead of releasing first.

\$\endgroup\$
  • \$\begingroup\$ I find standard layout unreadable. Fixed errors with copy and move assignment for resource (strange that MSVS doesn't care...). I never tried to work with base types... nor it was designed to do so. It is annoying that C++ works this way. About why int to count references... I doubt it will ever make int overflow int for ref counter so it is not a practical error. is_derived_from<> perhaps the encapsulation is preferred indeed, but I am not too certain how to properly address cases when keywords const/volatile are included. \$\endgroup\$ – ALX23z Oct 31 at 18:10
  • \$\begingroup\$ "The res_version adapter should virtually inherit resource, to avoid the diamond problem when subclasses inherit from more than one of these." I don't think so. If a subclass inherits resource version then you shouldn't make res_version to begin with; or better make a compatibility version that doesn't add anything anything if resource is a subclass. \$\endgroup\$ – ALX23z Oct 31 at 18:13
  • \$\begingroup\$ Wait... doesn't swap use std::move when possible? If it doesn't then it is a std error or the compiler's implementation of std fault. \$\endgroup\$ – ALX23z Oct 31 at 18:14
  • \$\begingroup\$ Ah yes, std::swap() requires MoveAssignable, so that's okay after all. \$\endgroup\$ – Toby Speight Oct 31 at 18:15
  • 1
    \$\begingroup\$ No, just as there isn't such a guarantee for std::atomic<int> - it's entirely dependent on the capabilities of the target you're compiling for, so the C++ standard can't mandate such choices. It's what you'd call a quality of implementation issue. I'd expect most platforms to use spinlocks rather than process mutexes if they don't have hardware atomic integer operations, though. \$\endgroup\$ – Toby Speight Oct 31 at 18:27
4
\$\begingroup\$

The only thing I see that is a mistake is the bool operator. Instead of:

operator bool   ()          const noexcept

You probably want:

explicit operator bool   ()          const noexcept

The explicit prevents the compiler from being able to automatically convert your object to bool as part of a comparison.

res_ptr<X>     data = getResPtr();

if (true == data) {
    // Not sue you want that auto conversion.
    // The standard shared_ptr does not want this implicit conversion
    // I am sure there are other cases where it can be out-converted where
    // you don't want that to happen.

         doSomeWorkWithBool(data); // If that function takes a bool
                                   // it auto converts before the call.
                                   // auto conversions are not usually what
                                   // you want.
}

// Note:
if (data) {
    // This still works as expected with the explicit
    // It is designed so that in a bool context like if()/while()/for() etc
    // the explicit conversion to bool is applied but in other contexts it
    // does not work any you need an explicit cast

    doSomeWorkWithBool(static_cast<bool>(data)); // Need an explict cast here
                                                 // if you use the `explicit`
                                                 // in the bool operator.
}
\$\endgroup\$
  • \$\begingroup\$ Thanks! I added explicit in the code (I don't update the question's version as per rules). Though, what you posted raised a question for me... res_ptr<int> does not compile because int doesn't inherit from resource. res_ptr<res_version<int>> needs to be used for this class to work with int. I didn't design res_ptr class to work with int so it might not be an issue but this design might be an issue in itself... \$\endgroup\$ – ALX23z Oct 31 at 17:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.