 | | | -none- | -none- 2007-08-15 - By Jonathan Wakely
Back
On 15/08/07, Joseph Artsimovich <joseph@(protected)> wrote:
> 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.
If it's used internally by library objects it's possibly not a
requirement. Users sharing Rows between threads already have to ensure
the Result object doesn't go out of scope in another thread, and Rows
already share state with the result that created them. If you follow
the doc's recommendation of not sharing Connections between threads,
then it doesn't matter.
This class will be significantly faster than a threadsafe
boost::shared_ptr / std::tr1::shared_ptr and if it's not shared that's
a win.
> 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)
100% agreed, swap is the way to go. I would also second the
recommendation to zero counted_ and refs_ after deleting them (even
though with the assign function shown above the detach would happen
after the memory allocation in the new pointer, which is the only
point that could throw.)
> 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
> }
Again, agreed. Although for an internal library component this might
not be needed, it's trivial to support. The template ctor and op= can
replace the existing ones taking T*. It will compile iff OT* is
convertible to T*
Swap also automatically protects against self-assignment. This will
access invalid memory:
int main() {
RefCountedPtr<int> a(new int());
return *(a=a);
}
If you change detach() to zero the pointers after delete then it will segfault.
> 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.
I would strongly advise against implicit conversion to T* or any other
"simple" type, including bool. This article has a good analysis of the
issue:
http://www.artima.com/cppsource/safebool.html
I think the latest Boost.SharedPtr actually uses a pointer to member
data, not member function. This trick solves far more problems than
the alternatives and creates far fewer.
With a safe bool there's no need for operator! ... the safe bool
conversion will be used and the builtin operator! for the type will be
used.
Jon
--
MySQL++ Mailing List
For list archives: http://lists.mysql.com/plusplus
To unsubscribe: http://lists.mysql.com/plusplus?unsub=mysql@(protected)
|
|
|