New shared pointer - revision 2

[Thiago Macieira]

Frerich Raabe wrote:
>> >                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.
>
>Ah okay, so 'd->obj = new KSharedData;' is okay?

According to Maksim on IRC, qAtomicSetPtr could be a memory fence, so I'd 
like to hear someone else's opinion on this. I don't understand why a 
"set pointer" function makes use of locking exchange instructions, so 
there may be a stronger reason.

What I suggested was d = new KSharedData; which looks ok to me.

>This is wrong. *this = other calls operator=( const KSharedPtr<T> & ),
> not operator=( T* ). Hence the newly allocated KSharedData won't be
> deallocated immediately (since the former assignment operator calls
> ref() on it's argument before deref()erencing).

The code path is this:
                        KSharedData *x = new KSharedData;
                        x = qAtomicSetPtr( &d, x );
                        rhs.d->ref.ref();
                        deref();
                        KSharedData *x = rhs.d;
                        x = qAtomicSetPtr( &d, x );

When deref() is called, the refcount on this (lhs) will be 1 and will, 
therefore, cause d to be deleted. If it weren't deleted, it would cause a 
memory leak because the last line sets d to rhs.d.

>
>> >                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;
>> >                }

Suppose d->ref == 2.
When you call deref(), it'll call d->ref.deref() which will make d->ref == 
1 and leave d != 0. At this point, you've detached from d, so you 
shouldn't write anymore to its contents.

Since d != 0, the if-statement is false. You then run 
qAtomicSetPtr( &d->obj, rhs ) which sets d->obj on the d you've just 
detached from.

> What would make sense is to use
> 'T *get() const;' tho

You can't do that without breaking constness. It would have to be const T* 
get() const;

>P.S.: You did overlook a much more grave problem (which I already fixed
>locally). Think about what happens when one does
>'KSharedPtr<int> p; KSharedPtr<int> q = p.copy();'

Oh, right.
-- 
  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/9545a5c8/attachment.sig>