Re: Eyeballs needed on new reference counted pointer template

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

Do you need help?

In addition to our free technical library, Pantek provides professional IT services to companies who utilize Open Source software: Learn more about Pantek's Technical Support offerings Place an order for 24/7/365 Technical Support here Learn more about Pantek, who we are, and what we do Chat with a Live Operator Related lists.mysql.com Links announce benchmarks bugs certification cluster commits dotnet gui-tools internals java maxdb myodbc mysql mysqldoc packagers perl plusplus replication win32 Request more information about Pantek

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=lists@pantek.com