[webkit-reviews] review denied: [Bug 41177] References for movable objects : [Attachment 63240] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 2 11:52:04 PDT 2010


Darin Adler <darin at apple.com> has denied Nathan Lawrence
<nlawrence at apple.com>'s request for review:
Bug 41177: References for movable objects
https://bugs.webkit.org/show_bug.cgi?id=41177

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	       struct {
> +		   int32_t tag;
> +		   JSCell* m_ptr;
> +	       } asPtr;

> +	       struct {
> +		   JSCell* m_ptr;
> +		   int32_t tag;
> +	       } asPtr;

Naming the pointer m_ptr instead of ptr or pointer seems to violate the
prevailing style here.

> +	   Removed unneeded marking.  We need to remove this marking in order
to have
> +	   MarkStack::append take references for updating movable objects.
> +	   https://bugs.webkit.org/show_bug.cgi?id=41177

This answers why we need to remove the marking, but doesn't say why the marking
is unneeded.

>  void JSValueWrapper::JSObjectMark(void *data)
>  {
> -    JSValueWrapper* ptr = (JSValueWrapper*)data;
> -    if (ptr)
> -    {
> -	   // This results in recursive marking but will be otherwise safe and
correct.
> -	   // We claim the array vptr is 0 because we don't have access to it
here, and
> -	   // claiming 0 is functionally harmless -- it merely means that we
can't
> -	   // devirtualise marking of arrays when recursing from this point.
> -	   MarkStack markStack(0);
> -	   markStack.append(ptr->fValue.get());
> -	   markStack.drain();
> -    }
>  }

It's a little strange to leave this function around with an argument, but not
doing any work. I think the comment about why marking is unneeded could
probably go here in the code rather than just in the change log.

review- because I think m_ptr is not the right way to name that


More information about the webkit-reviews mailing list