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.
