Bug 29822 – NotNullPassRefPtr: smart pointer optimized for passing references that are not null

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Geoffrey Garen]

Patch coming.

[Geoffrey Garen]

Created attachment 40253 [details]
patch

[Darin Adler]

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.

[Geoffrey Garen]

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

[Geoffrey Garen]

Committed revision 48836.