1
\$\begingroup\$

An alternative to shared_ptr to minimize compile time. Intrusive. Reference count changes are not thread safe.

(#include<memory> pulls in 17k lines of code)

This satisfies my needs, but I'm curious how it could be improved (without adding any dependencies).

It's not particularly good at handling incomplete types (ref<T>::get cannot be used on an incomplete type). Is it possible to improve that?

// Derive from this to be refcounted.
struct refcount {
  int rc = 0;
  virtual ~refcount() { }
};

void queued_delete(refcount*);

template<class T>
class ref {

public:

  ref() { }

  ref(T* obj) : obj(obj) {
    if(obj) {
      obj->rc++;
    }
  }

  ~ref() {
    if(obj) {
      if(--(obj->rc) == 0) {
        queued_delete(obj);
      }
    }
  }

  ref(const ref& r) {
    if(r.obj) {
      obj = r.obj;
      obj->rc++;
    }
  }

  template<class T2>
  ref(const ref<T2>& r) {
    if(r.get()) {
      obj = r.get();
      obj->rc++;
    }
  }

  ref& operator=(const ref& r) {
    ref(r).swap(*this);
    return *this;
  }

  template<class T2>
  ref& operator=(const ref<T2>& r) {
    ref(r).swap(*this);
    return *this;
  }

  T* operator->() { return static_cast<T*>(obj); }
  const T* operator->() const { return static_cast<const T*>(obj); }
  T& operator*() { return *get(); }
  T& operator*() const { return *get(); }

  T* get() const { return static_cast<T*>(obj); }

  unsigned long id() const { return (unsigned long) obj; }

  void swap(ref& p) {
    auto tmp = obj;
    obj = p.obj;
    p.obj = tmp;
  }

  explicit operator bool() const {
    return obj;
  }

  bool operator==(const ref& other) const {
    return obj == other.obj;
  }

  bool operator!=(const ref& other) const {
    return obj != other.obj;
  }

  template<class T2>
  ref<T2> cast() {
    return ref<T2>(dynamic_cast<T2*>(obj));
  }

private:
  refcount *obj = nullptr;
};

Optional deletion which can handle deep nesting (can also do this with shared_ptr using a custom deleter):

// Ensure deeply nested data structures can
// be deleted without running out of stack.
void queued_delete(refcount* p) {

  static bool deleting = false;
  static std::vector<refcount*> stack;

  stack.push_back(p);

  if(!deleting) {
    deleting = true;
    while(stack.size()) {
      auto top = stack.back();
      stack.pop_back();
      delete top;
    }
    deleting = false;
  }
}
\$\endgroup\$
2
\$\begingroup\$

You didn't post a complete header file (no include-guard/#pragma once), so it's unclear whether you also omitted a namespace Taylor { somewhere in there. But you should definitely use a namespace around a name as common as ref.


  virtual ~refcount() { }

Nit: You could use = default; instead of { } here, and you might get slightly better codegen on some compilers. (Virtual destructors are never actually "trivial," unfortunately, but getting the compiler to write this quasi-trivial code for you can't hurt.)


I notice that refcount->rc is public, not private. Was that a design decision?

It occurs to me that the proper name for what you're calling refcount is actually refcounted. The refcount, physically, is the data member rc. A type which derives from your base class is a refcounted type.


ref(T* obj) : obj(obj) {
  if(obj) {
    obj->rc++;
  }
}

I would write this as

explicit ref(T *obj) : obj_(obj) {
    if (obj_ != nullptr) {
        obj_->rc += 1;
    }
}

Notice the whitespace edits, the use of some kind of sigil for data members (to avoid having two different variables named obj in scope), my preference for += 1 when the increment is a stand-alone statement, the explicit comparison against nullptr in place of contextual-conversion-to-bool, and the addition of explicit. Non-explicit constructors permit implicit conversions, e.g.

struct Widget : public refcount { int data = 42; };

void print_data_of(ref<Widget> x) { std::cout << x->data << "\n"; }

int main() {
    Widget *p = new Widget;
    print_data_of(p);
    delete p;  // OOPS! Double delete!
}

It would be better (IMO of course) if this code did not compile.


template<class T2>
ref(const ref<T2>& r) {
  if(r.get()) {
    obj = r.get();
    obj->rc++;
  }
}

This is extremely sketchy. Consider:

struct Widget : public refcount { int data = 42; };
struct Gadget : public refcount { int x = -1; int data = 42; };
int main() {
    ref<Widget> w = new Widget;  // OK...
    ref<Gadget> v = w;           // ...sketchy...
    std::cout << v->data << "\n";  // OOPS! Prints "-1", not "42"
}

This constructor should be either completely removed, or else constrained (using enable_if, C++2a requires, or some other trickery) so that it participates in overload resolution only when T2* would be convertible to T*.

One easy way to mostly-fix this would be to simply add an assertion:

template<class T2>
ref(const ref<T2>& r) {
    static_assert(std::is_convertible_v<T2*, T*>);
    if (r != nullptr) {
        obj_ = r.get();
        obj_->rc += 1;
    }
}

Here we're lying to the library (e.g. std::is_constructible_v<ref<Widget>, ref<Gadget>> will still be true), but at least we prevent the client programmer from accidentally writing a program like the test case above.


Another way to fix the issue would be to rely on pointer-assignment to do the check for us. Instead of refcount *obj_;, let's make our data member look like T *obj_;. Then we can write

template<class T2>
ref(const ref<T2>& r) {
    if (r != nullptr) {
        obj_ = r.obj_;  //HERE
        obj_->rc += 1;
    }
}

And then if T2* isn't convertible to T*, we'll get an error on the line marked //HERE.

Incidentally, this also solves your problem with ref<T>::get() and incomplete types.


T& operator*() { return *get(); }
T& operator*() const { return *get(); }

You don't need both versions of the function, since they do exactly the same thing. Just write the const version. (Dereferencing a pointer doesn't need to modify the pointer, remember. Const is a contract.)

T* operator->() { return static_cast<T*>(obj); }
const T* operator->() const { return static_cast<const T*>(obj); }

And in this case you've got the two versions doing different things, but that's still wrong, because they shouldn't be doing different things! Dereferencing a pointer doesn't need to modify the pointer. What you meant in both cases was simply

T* get() const { return static_cast<T*>(obj); }
T* operator->() const { return get(); }
T& operator*() const { return *get(); }

Your void swap(ref& p) should be noexcept — just like your move-constructor, which I guess you didn't write. (You should do some move semantics here!)

I recommend implementing your swap as a one-liner: std::swap(obj_, rhs.obj_);.

A member swap function will not be picked up by any standard library algorithms. If you want your swap to be actually used, you'll need to provide an ADL swap, like this:

friend void swap(ref& a, ref& b) noexcept { a.swap(b); }

Your queued_delete is interesting. It's misnamed, in that its deletions are stacked (LIFO), not queued (FIFO). I don't know if that makes a difference to performance or anything like that, in practice.

It's also thread-unsafe, which is not clear from your description/documentation. In standard C++, we can write

int main() {
    std::shared_ptr<Widget> p(new Widget);
    std::thread([q = p]() {
        q = nullptr;
    }).detach();
    p = nullptr;
}

and be guaranteed that the writes to the refcount shared by p and q won't race with each other. Your documentation clearly states that we can't do that with your ref<Widget>. But what is surprising to me is that we also can't do the following!

int main() {
    ref<Widget> p(new Widget);
    std::thread([]() {
        ref<Gadget> q(new Gadget);
        q = nullptr;
    }).detach();
    p = nullptr;
}

Here, p.obj->rc and q.obj->rc are completely different objects, so there's no race there; but then they each call into queued_delete and try to write to stack, and those writes race with each other. So your ref is completely unsafe for use within a multi-threaded environment, even if you never share any objects between threads.

In theory I guess you could "fix" this by replacing the storage class static with thread_local everywhere it appears; but, please don't do that.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the excellent review! \$\endgroup\$ – Taylor Jul 24 '19 at 23:49
  • \$\begingroup\$ When I try to change obj to be a T*, I get errors because I can't static_cast<refcount*>(obj) when T is an incomplete type. shared_ptr doesn't have this issue because of the separately allocated control block. Not sure how to fix it. \$\endgroup\$ – Taylor Jul 24 '19 at 23:57
  • \$\begingroup\$ "Was that a design decision?" actually, yes, because I'm thinking of having these refcounted objects also managed through a C API so I can use them from Swift. Perhaps a cleaner approach would be to have retain and release functions on refcounted \$\endgroup\$ – Taylor Jul 25 '19 at 0:10
  • \$\begingroup\$ Okay, true, if obj points to a T and T is incomplete, then the compiler won't know how to increment or decrement obj->refcount (because it won't know where the refcount member is located within the T). But my suggestion does change your pain point from "I can't call ref<T>::get() when T is incomplete" to "I can't create, copy or destroy ref<T> when T is incomplete." I think that's at least a slightly better pain to have. \$\endgroup\$ – Quuxplusone Jul 25 '19 at 0:33
  • \$\begingroup\$ I managed to make it work with a T* by adding to ref<T> a pointer to a function which converts the T* to refcounted*. \$\endgroup\$ – Taylor Jul 25 '19 at 6:56

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.