There are several problems with your RefCountedPointer template:
1. It's not thread-safe. You can't reference a single object from multiple
threads. To be able to do so, incrementing and decrementing the reference
counter must be atomic.
2. It's not exception-safe, as was already pointed out. In assign() you first
call detach(), and then allocate memory. Even if you would allocate memory
before detach(), that would not solve the problem, as you would still leak
the argument to assign() in case memory allocation fails.
The correct way to implement assign is:
void assign(T* c)
{
ThisType(c).swap(*this);
}
Where swap() just swaps the pointers. The same implementation can be used for
assign(const ThisType& c)
3. It should be possible to do this:
class A {};
class B : public A {};
RefCountedPointer<B> pb(new B);
RefCountedPointer<A> pa(pb);
To achieve this, you need to create a template constructor and assignment
operator, like this:
template<typename T>
template<typename OT>
RefCountedPointer<T>::RefCounterPointer(const RefCountedPointer<OT>&
other)
{
// the usual stuff here
}
4. Implicit convertion to bool is dangerous. It allows code like this to
compile:
RefCountedPointer<A> pa;
int a = pa;
Even worse, because you haven't provided comparison operators, comparing two
smart pointers will now involve converting both of them to bool!
Some books suggest providing an implicit conversion to a pointer-to-member
instead of bool. That's slightly better, but it doesn't solve all the
problems. My opinion is that you either don't provide implicit conversion at
all, or you provide implicit conversion to T*, which is also dangerous, but
in practice I never had troubles with this approach.
Regards,
Joseph Artsimovich
