30 messages in com.mysql.lists.plusplusRe: Eyeballs needed on new reference ...
FromSent OnAttachments
Warren Young14 Aug 2007 23:26 
Chris Frey15 Aug 2007 01:14 
Alex Burton15 Aug 2007 01:42 
Joseph Artsimovich15 Aug 2007 01:56 
Joel Fielder15 Aug 2007 02:29 
Robert Mecklenburg15 Aug 2007 07:47 
Graham Reitz15 Aug 2007 08:38 
Graham Reitz15 Aug 2007 08:47 
Graham Reitz15 Aug 2007 09:03 
Jonathan Wakely15 Aug 2007 15:06 
Warren Young16 Aug 2007 01:55 
Warren Young16 Aug 2007 02:23 
Warren Young16 Aug 2007 02:31 
Warren Young16 Aug 2007 02:57 
Warren Young16 Aug 2007 03:08 
Warren Young16 Aug 2007 03:11 
Jonathan Wakely17 Aug 2007 17:54 
Jonathan Wakely17 Aug 2007 18:00 
Warren Young20 Aug 2007 12:09 
Warren Young29 Nov 2007 00:04 
Joseph Artsimovich29 Nov 2007 04:20 
Warren Young29 Nov 2007 05:48 
Jonathan Wakely29 Nov 2007 17:40 
Jonathan Wakely29 Nov 2007 17:58 
Jonathan Wakely29 Nov 2007 18:15 
Warren Young30 Nov 2007 22:24 
Warren Young30 Nov 2007 22:35 
Warren Young30 Nov 2007 22:38 
Warren Young17 Dec 2007 05:53 
Jonathan Wakely17 Dec 2007 12:39 
Subject:Re: Eyeballs needed on new reference counted pointer template
From:Jonathan Wakely (jona@gmail.com)
Date:08/15/2007 03:06:11 PM
List:com.mysql.lists.plusplus

On 15/08/07, Joseph Artsimovich <jos@tts.lt> 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