[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