One more shared pointer

Question

Here's a (hopefully) better shared pointer than the previous one. The improvement is that now it should also accept function and lambda objects. Should work fine with threads.

#pragma once
#ifndef LIGHTPTR_HPP
# define LIGHTPTR_HPP

#include <cassert>

#include <atomic>

#include <memory>

#include <utility>

#include <type_traits>

namespace detail
{
  using counter_type = unsigned;

  using atomic_type = ::std::atomic<counter_type>;

  template <typename T>
  using deleter_type = void (*)(T*);

  template <typename U>
  struct ref_type
  {
    using type = U&;
  };

  template <>
  struct ref_type<void>
  {
    using type = void;
  };
}

template <typename T>
struct light_ptr
{
  template <typename U, typename V>
  struct deletion_type
  {
    using type = V;
  };

  template <typename U, typename V>
  struct deletion_type<U[], V>
  {
    using type = V[];
  };

  template <typename U, typename V, ::std::size_t N>
  struct deletion_type<U[N], V>
  {
    using type = V[];
  };

  template <typename U>
  struct remove_array
  {
    using type = U;
  };

  template <typename U>
  struct remove_array<U[]>
  {
    using type = U;
  };

  template <typename U, ::std::size_t N>
  struct remove_array<U[N]>
  {
    using type = U;
  };

  using element_type = typename remove_array<T>::type;

  using deleter_type = detail::deleter_type<element_type>;

  struct counter_base
  {
    using invoker_type = void (*)(counter_base*, element_type*);

    explicit counter_base(detail::counter_type c,
      invoker_type invoker) noexcept :
      counter_(c),
      invoker_(invoker)
    {
    }

    template <typename U>
    typename ::std::enable_if<!::std::is_void<U>{}>::type
    dec_ref(U* const ptr)
    {
      if (detail::counter_type(1) ==
        counter_.fetch_sub(detail::counter_type(1),
          ::std::memory_order_relaxed))
      {
        typedef char type_must_be_complete[sizeof(U) ? 1 : -1];
        (void)sizeof(type_must_be_complete);
        invoker_(this, ptr);
      }
      // else do nothing
    }

    template <typename U>
    typename ::std::enable_if<::std::is_void<U>{}>::type
    dec_ref(U* const ptr)
    {
      if (detail::counter_type(1) ==
        counter_.fetch_sub(detail::counter_type(1),
          ::std::memory_order_relaxed))
      {
        invoker_(this, ptr);
      }
      // else do nothing
    }

    void inc_ref() noexcept
    {
      //assert(counter_ptr);
      counter_.fetch_add(detail::counter_type(1),
        ::std::memory_order_relaxed);
    }

    detail::atomic_type counter_{};

    invoker_type invoker_;
  };

  template <typename U>
  struct counter : counter_base
  {
    explicit counter(detail::counter_type const c, U&& d) :
      counter_base(c, invoker),
      d_(::std::forward<U>(d))
    {
    }

  private:
    static void invoker(counter_base* const ptr, element_type* e)
    {
      auto const c(static_cast<counter<U>*>(ptr));

      typename ::std::decay<U>::type const d(::std::move(c->d_));

      delete c;

      d(e);
    }

  private:
    typename ::std::decay<U>::type const d_;
  };

  light_ptr() = default;

  template <typename U>
  explicit light_ptr(U* p)
  {
    reset(p);
  }

  template <typename U, typename D>
  explicit light_ptr(U* p, D&& d)
  {
    reset(p, ::std::forward<D>(d));
  }

  ~light_ptr()
  {
    if (counter_)
    {
      counter_->dec_ref(ptr_);
    }
    // else do nothing
  }

  light_ptr(light_ptr const& other) { *this = other; }

  light_ptr(light_ptr&& other) noexcept { *this = ::std::move(other); }

  light_ptr& operator=(light_ptr const& rhs)
  {
    if (*this != rhs)
    {
      if (counter_)
      {
        counter_->dec_ref(ptr_);
      }
      // else do nothing

      counter_ = rhs.counter_;
      ptr_ = rhs.ptr_;

      if (counter_)
      {
        counter_->inc_ref();
      }
      // else do nothing
    }
    // else do nothing

    return *this;
  }

  light_ptr& operator=(light_ptr&& rhs) noexcept
  {
    counter_ = rhs.counter_;
    ptr_ = rhs.ptr_;

    rhs.counter_ = nullptr;
    rhs.ptr_ = nullptr;

    return *this;
  }

  light_ptr& operator=(::std::nullptr_t const) noexcept { reset(); }

  bool operator<(light_ptr const& rhs) const noexcept
  {
    return get() < rhs.get();
  }

  bool operator==(light_ptr const& rhs) const noexcept
  {
    return counter_ == rhs.counter_;
  }

  bool operator!=(light_ptr const& rhs) const noexcept
  {
    return !operator==(rhs);
  }

  bool operator==(::std::nullptr_t const) const noexcept
  {
    return !ptr_;
  }

  bool operator!=(::std::nullptr_t const) const noexcept
  {
    return ptr_;
  }

  explicit operator bool() const noexcept { return ptr_; }

  typename detail::ref_type<T>::type
  operator*() const noexcept
  {
    return *static_cast<T*>(static_cast<void*>(ptr_));
  }

  T* operator->() const noexcept
  {
    return static_cast<T*>(static_cast<void*>(ptr_));
  }

  element_type* get() const noexcept { return ptr_; }

  void reset() { reset(nullptr); }

  void reset(::std::nullptr_t const)
  {
    if (counter_)
    {
      counter_->dec_ref(ptr_);

      counter_ = {};
    }
    // else do nothing

    ptr_ = {};
  }

  template <typename U>
  void reset(U* const p)
  {
    reset(p, [](element_type* const p) {
      ::std::default_delete<typename deletion_type<T, U>::type>()(
        static_cast<U*>(p));
      });
  }

  template <typename U, typename D>
  void reset(U* const p, D&& d)
  {
    if (counter_)
    {
      counter_->dec_ref(ptr_);
    }
    // else do nothing

    counter_ = new counter<D>(detail::counter_type(1), ::std::forward<D>(d));

    ptr_ = p;
  }

  void swap(light_ptr& other) noexcept
  {
    ::std::swap(counter_, other.counter_);
    ::std::swap(ptr_, other.ptr_);
  }

  bool unique() const noexcept
  {
    return detail::counter_type(1) == use_count();
  }

  detail::counter_type use_count() const noexcept
  {
    return counter_ ?
      counter_->counter_.load(::std::memory_order_relaxed) :
      detail::counter_type{};
  }

private:
  counter_base* counter_{};

  element_type* ptr_{};
};

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

namespace std
{
  template <typename T>
  struct hash<::generic::light_ptr<T> >
  {
    size_t operator()(::generic::light_ptr<T> const& l) const noexcept
    {
      return hash<typename ::generic::light_ptr<T>::element_type*>()(l.get());
    }
  };
}

#endif // LIGHTPTR_HPP

Usage is the same as for my previous shared pointer. Improvements?

EDIT: Loki threads issue is wrong. Look here for explanation. His example is one boost defines as UB.

You mean, there's too much line spacing? Just a personal preference. — user1095108
– user1095108, Commented May 13, 2014 at 21:35
No. My point in it being broken for threading still holds. You have not proven otherwise and I provide a compelling example of how easily it will break. Fortunately for us the std::shared_ptr is not broken in shared environments (Your link). — Loki Astari
– Loki Astari, Commented May 14, 2014 at 5:08
Your example shows canonical UB from the link I've provided (reset from one thread and copy from another, which is read/write access from different threads, which results in UB). — user1095108
– user1095108, Commented May 14, 2014 at 7:10
@LokiAstari Also, read this: stackoverflow.com/questions/11491812/… . — user1095108
– user1095108, Commented May 14, 2014 at 7:28

Loki Astari · Accepted Answer · 2014-05-13 22:11:39Z

Lets the obligatory (this is not a good idea) out of the way.

I don't like you cramming all the code together into a single header definition. It makes it hard to see the interface. Yes the standard library do it this way but they are so heavily documented that they don't need to break it apart. Your code is not documented so I need to be able to distinguish the interface.

Its not hard to break the declaration and the definition into different parts of the file:

class MyClass
{
    public:
        void myInt1();
        int  myInt2();
    private:
        Data data;
};

inline void MyClass::myInt1()
{
}

inline int MyClass::myInt2()
{
    return 0;
}

Basically I want to be able to see the class declaration in one screen (so I can see what variables need to be initialized when doing a review). I can also quickly gauge and see the public interface (when using the class).

Lets also put all the constructors together. So I don't have to search in multiple parts of your class for the different constructors.

  light_ptr() = default;

  template <typename U>
  explicit light_ptr(U* p);

  template <typename U, typename D>
  explicit light_ptr(U* p, D&& d);

  ~light_ptr();

  light_ptr(light_ptr const& other);

  light_ptr(light_ptr&& other) noexcept;

  light_ptr& operator=(light_ptr const& rhs);

  light_ptr& operator=(light_ptr&& rhs) noexcept;

  light_ptr& operator=(::std::nullptr_t const) noexcept;

For some reason your destructor is in the middle. So you have some constructors on either side.

Copy constructor:

light_ptr(light_ptr const& other) { *this = other; }

That's not the copy and swap idiom (seems to be the inverse). May throw a few people. If you are not going to use standard idioms them you should have a go at explaining why.

Not sure this is helpeful:

// else do nothing

I can see that there is no else. You are just asking for trouble when the code goes out of sync with the comments. Comments are for describing why you are doing something. There is no point in describing the code with comments the code should be readable by-itself the code describes what it is doing.

You go to all the effort of making the counter inside counter_base atomic. But this is a complete false sense of security as the counter_ variable inside light_ptr is not atomic or protected in any way. Also there is no checking of the counter when accessing the element so you can just as easily hand off an invalid pointer.

// Main Thread.
light_ptr<MyType>   data(new MyType);


     Thread1                                           Thread2
     data.reset();                                     light_ptr<MyType>  tmp(data);

     void reset(::std::nullptr_t const)
     {
         if (counter_)
         {
             counter_->dec_ref(ptr_);

             // Thread is swapped out here.
             // counter is 0 and ptr has been de-allocated.
             // But has not been nulled out:


                                                       light_ptr& operator=(light_ptr const& rhs)
                                                       {
                                                           if (*this != rhs)
                                                           {
                                                             // This side is NULL so this condition can be ignored.
                                                             if (counter_)
                                                             {
                                                               counter_->dec_ref(ptr_);
                                                             }


                                                             // Use the counter from (data) which happens to currently be 0 but is valid.
                                                             counter_ = rhs.counter_;
                                                             // Get a copy of rhs pointer (which is invalid as it has already been free'ed)
                                                             ptr_ = rhs.ptr_;

                                                             // Now we increment the counter. As the pointer is still valid.
                                                             if (counter_)
                                                             {
                                                               counter_->inc_ref();
                                                             }
                                                           }
                                                           // So we now have a count of 1. With a pointer to an invalid piece of memoy.
                                                           return *this;
                                                       }

             // Reset the local counter.
             counter_ = {};
         }

         // Reset the local ptr.
         ptr_ = {};
     }

Why the difference here:

  bool operator<(light_ptr const& rhs) const noexcept
  {
    return get() < rhs.get();         // Why are we using the ptr here
  }

  bool operator==(light_ptr const& rhs) const noexcept
  {
    return counter_ == rhs.counter_;  // And the counter here?
  }

I would be more consistent.

 Example of inconistent behavior:

 std::set<light_ptr<MyType>>  set;
 MyType*   data = new MyType;
 set.emplace(data);
 set.emplace(data);
 std::cout << set.size() << "\n";  // Prints 1
                                   // As the values are considered euqal.
                                   // As we use operator< for comparison
                                   // and equality.

 std::unordered_set<light_ptr<MyType>>  set1;
 set1.emplace(data);
 set1.emplace(data);
 std::cout << set1.size() << "\n"; // Prints 2
                                   // As the values are not considered euqal.
                                   // They hash to the same bucket.
                                   // But because they have unique counter_ objects
                                   // They are not equal and thus result in different
                                   // results.

Maybe the difference is not a big issue as you are going to crash with a double delete anyway. But you break the principle of least surprise by doing it differently.

for the relational operators: one can initialize 2 shared pointers with the same pointer - it is a bug, as a double delete will occur. but the shared pointers should not compare equal, in my opinion. — user1095108
– user1095108, Commented May 13, 2014 at 21:44
@user1095108: And they would not if you used counter in both cases. The trouble with your implementation is that they will be compared equal when placed in an associative container (which just uses operator<) So you will have one object copy over the other. — Loki Astari
– Loki Astari, Commented May 13, 2014 at 21:49
Imho it doesn't matter how to smart-pointers pointing to the same object which do not cooperate compare: That is a bug there already. — Deduplicator
– Deduplicator, Commented May 13, 2014 at 21:52
@LokiAstari for the threads: can you rework your example: I don't see where reset(nullptr) is called. — user1095108
– user1095108, Commented May 13, 2014 at 21:52
Comment to include guards: If you use both, stack them the other way around for better effect. — Deduplicator
– Deduplicator, Commented May 13, 2014 at 21:53

Stack Exchange Network

One more shared pointer

1 Answer 1

You must log in to answer this question.

Linked

Hot Network Questions

One more shared pointer

1 Answer 1

You must log in to answer this question.

Linked

Related

Hot Network Questions