Bug 29822 - NotNullPassRefPtr: smart pointer optimized for passing references that are not null
Summary: NotNullPassRefPtr: smart pointer optimized for passing references that are no...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-28 13:18 PDT by Geoffrey Garen
Modified: 2009-09-28 14:47 PDT (History)
0 users

See Also:


Attachments
patch (95.01 KB, patch)
2009-09-28 13:35 PDT, Geoffrey Garen
darin: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2009-09-28 13:18:34 PDT
Patch coming.
Comment 1 Geoffrey Garen 2009-09-28 13:35:07 PDT
Created attachment 40253 [details]
patch
Comment 2 Darin Adler 2009-09-28 13:41:47 PDT
Comment on attachment 40253 [details]
patch

NonNull instead of NotNull?

> +        (JSC::::JSCallbackObject):

> +        (WTF::::RefPtr):
> +        (WTF::::operator):

Bad lines written by the prepare-ChangeLog script, but you left these in. Yuck.

> +    // FIXME: NotNullPassRefPtr should just inherit from PassRefPtr. However,
> +    // GCC's optimizer gets inexplicably confused by inheritance in this case.

"confused" is vague here. I think it would be better if you were more specific about what it does not optimize.

> +        ALWAYS_INLINE ~NotNullPassRefPtr() { derefIfNotNull(m_ptr); }

I think you need a comment somewhere clarifying that NotNull does not mean it's never null! And when it can and cannot be null. If you had such a comment I would have reviewed differently.

> +        void clear() { if (T* ptr = m_ptr) ptr->deref(); m_ptr = 0; }

Should this perhaps ASSERT instead of checking?

> +        T* releaseRef() const { T* tmp = m_ptr; m_ptr = 0; return tmp; }

Maybe this should assert m_ptr.

> +        T& operator*() const { return *m_ptr; }
> +        T* operator->() const { return m_ptr; }

Shouldn't these assert m_ptr?

> +        bool operator!() const { return !m_ptr; }
> +
> +        // This conversion operator allows implicit conversion to bool but not to other integer types.
> +        typedef T* (NotNullPassRefPtr::*UnspecifiedBoolType);
> +        operator UnspecifiedBoolType() const { return m_ptr ? &NotNullPassRefPtr::m_ptr : 0; }

Do we need these at all?

>          return p.get();
>      }
> -
> +    
>  } // namespace WTF

Don't add the whitespace, please.
Comment 3 Geoffrey Garen 2009-09-28 14:14:55 PDT
> NonNull instead of NotNull?

OK.

> > +        (JSC::::JSCallbackObject):
> 
> > +        (WTF::::RefPtr):
> > +        (WTF::::operator):
> 
> Bad lines written by the prepare-ChangeLog script, but you left these in. Yuck.

Fixed.

> > +    // FIXME: NotNullPassRefPtr should just inherit from PassRefPtr. However,
> > +    // GCC's optimizer gets inexplicably confused by inheritance in this case.
> 
> "confused" is vague here. I think it would be better if you were more specific
> about what it does not optimize.

Changed to:

    // FIXME: NonNullPassRefPtr could just inherit from PassRefPtr. However,
    // if we use inheritance, GCC's optimizer fails to realize that destruction
    // of a released NonNullPassRefPtr is a no-op. So, for now, just copy the
    // most important code from PassRefPtr.

> > +        ALWAYS_INLINE ~NotNullPassRefPtr() { derefIfNotNull(m_ptr); }
> 
> I think you need a comment somewhere clarifying that NotNull does not mean it's
> never null! And when it can and cannot be null. If you had such a comment I
> would have reviewed differently.
> 
> > +        void clear() { if (T* ptr = m_ptr) ptr->deref(); m_ptr = 0; }
> 
> Should this perhaps ASSERT instead of checking?
> 
> > +        T* releaseRef() const { T* tmp = m_ptr; m_ptr = 0; return tmp; }
> 
> Maybe this should assert m_ptr.
> 
> > +        T& operator*() const { return *m_ptr; }
> > +        T* operator->() const { return m_ptr; }
> 
> Shouldn't these assert m_ptr?
> 
> > +        bool operator!() const { return !m_ptr; }
> > +
> > +        // This conversion operator allows implicit conversion to bool but not to other integer types.
> > +        typedef T* (NotNullPassRefPtr::*UnspecifiedBoolType);
> > +        operator UnspecifiedBoolType() const { return m_ptr ? &NotNullPassRefPtr::m_ptr : 0; }
> 
> Do we need these at all?
> 
> >          return p.get();
> >      }

Added:

    // NonNullPassRefPtr: Optimized for passing non-null pointers. A NonNullPassRefPtr
    // begins life non-null, and can only become null through a call to releaseRef()
    // or clear().

Removed the boolean operator, since it seems to go against the spirit of the pointer. I decided to leave out extra ASSERTs, though. Might be worth adding in the future if we come up with an optimization that would depend on them.

> > -
> > +    
> >  } // namespace WTF
> 
> Don't add the whitespace, please.

Fixed.
Comment 4 Geoffrey Garen 2009-09-28 14:47:52 PDT
Committed revision 48836.