New shared pointer - revision 2

[Frerich Raabe]

On Saturday 10 September 2005 01:48, Thiago Macieira wrote:
> 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++.

I know that it's valid, but it's not a good idea. Leaving aside that KShared 
is a baseclass, gcc will give you warnings if you don't make the destructor 
virtual because some classes (KSycocaEntry, for instance) have virtual 
functions but don't have a virtual destructor. Since KShared had a virtual 
destructor as well, and since this is just a helper to ease porting (so it 
should try to resemble the old behaviour), the virtual should be there.

> >                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?

> >                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?

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).

> >                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.

It is.

> 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.

This is wrong - rhs is a T*, not a KSharedData* - there is no write access to 
d itself at all, it doesn't get overwritten at all.

> >                T *get()
> >                {
> >                        return d->obj;
> >                }
>
> Can I suggest a const T *get() const function?

This doesn't make a difference, since get() is only a helper function which 
you should use if you're desperate, there's not much point in trying to try 
making this safer to use. What would make sense is to use 'T *get() const;' 
tho, so that you can do such evil things even on const KSharedPtr objects 
(there's not much point in restrictions for such things).

> 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.

I'm indifferent on this.

- Frerich

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();'
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20050910/5c4d8e6a/attachment.sig>