[webkit-reviews] review granted: [Bug 29822] NotNullPassRefPtr: smart pointer optimized for passing references that are not null : [Attachment 40253] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Geoffrey Garen <ggaren at apple.com>'s
request for review:
Bug 29822: NotNullPassRefPtr: smart pointer optimized for passing references
that are not null
https://bugs.webkit.org/show_bug.cgi?id=29822

Attachment 40253: patch
https://bugs.webkit.org/attachment.cgi?id=40253&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.