New shared pointer - revision 2

[Thiago Macieira]

Frerich Raabe wrote:
>struct KDECORE_EXPORT KShared
>{
>        virtual ~KShared() {}
>};

The destructor here is unnecessary and its presence will cause an 
out-of-line empty destructor, a virtual table and the typeinfo 
information to be emitted, which is unnecessary.

Simply leave the struct empty. It's valid in C++.

>                KSharedPtr( T *obj = 0 )
>                {
>                        KSharedData *x = new KSharedData;
>                        x = qAtomicSetPtr( &d, x );
>                        d->ref = 1;
>                        obj = qAtomicSetPtr( &d->obj, obj );
>                }

I think you're overkilling here with qAtomicSetPtr. There's no 
thread-safety issue with the KSharedData variables d and x because you've 
just allocated it.

>                KSharedPtr( const KSharedPtr<T> &other )
>                {
>                        KSharedData *x = new KSharedData;
>                        x = qAtomicSetPtr( &d, x );
>                        *this = other;
>                }

Again the unnecessary calls. Also, you're allocating one KSharedData 
object only to deallocate it in operator=() when it calls deref(). Can't 
you optimise it to avoid allocating at all?

For instance, make deref() detect if d == 0?

>                KSharedPtr<T> &operator=( T *rhs )
>                {
>                        deref();
>                        if ( !d ) {
>                                KSharedData *x = new KSharedData;
>                                x = qAtomicSetPtr( &d, x );
>                        }
>                        rhs = qAtomicSetPtr( &d->obj, rhs );
>                        d->ref = 1;
>                        return *this;
>                }

I don't think this is correct.

If the object is holding an object somewhere with refcount > 1, then 
deref() won't do anything and d != 0. Therefore, you won't create a new 
KSharedData and will overwrite the existing (and shared!) one with rhs.

I'd rewrite this function as:
  deref()
  d = new KSharedData;
  qAtomicSetPtr( &d->obj, rhs );
  d->ref = 1;
  return *this;

>                T *get()
>                {
>                        return d->obj;
>                }

Can I suggest a const T *get() const function?

May I also suggest that you don't use a pointer to QString in the class's 
apidox? QString is already implicitly-shared and pointers to it are 
almost always wrong, so we should not provide incentive to use them. 
(it's ok in the test code). Use a dummy class like FooBar.

-- 
  Thiago Macieira  -  thiago (AT) macieira.info - thiago (AT) kde.org
    PGP/GPG: 0x6EF45358; fingerprint:
    E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358

4. And æfter se scieppend ingelogode, he wrát "cenn", ac eala! se 
rihtendgesamnung andswarode "cenn: ne wát hú cennan 'eall'. Ástynt."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20050909/51051476/attachment.sig>