[webkit-reviews] review granted: [Bug 209043] Should not use variable-length-array (VLA) : [Attachment 393579] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 209043: Should not use variable-length-array (VLA)
https://bugs.webkit.org/show_bug.cgi?id=209043

Attachment 393579: Patch

https://bugs.webkit.org/attachment.cgi?id=393579&action=review




--- Comment #6 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 393579
  --> https://bugs.webkit.org/attachment.cgi?id=393579
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393579&action=review

r=me with one concern about JSAPIValueWrapper on !CPU(ADDRESS64) targets.

> Source/JavaScriptCore/API/MarkedJSValueRefArray.cpp:60
> +	   visitor.appendUnbarriered(jsCell); // We should mark the wrapper
itself to keep JSValueRef live.

I didn't quite understand what this comment meant in the first place until I
read JSC::JSValue toJS(JSC::JSGlobalObject* globalObject, JSValueRef v).

>From my reading of the code, I see that the jsCell here may be a
JSAPIValueWrapper which is itself a JSCell.  Hence, we should visit the
JSAPIValueWrapper itself, and that means appending the jsCell here should do
the right thing regardless of whether the jsCell is a JSAPIValueWrapper or some
other JSCell.

That said, JSAPIValueWrapper does not appear to have a visitChildren()
function.  Hence, I don't see how its m_value will get visited.  Am I missing
something?

If the above is accurate, I think we don't need this comment.  We do need to
look into how the JSAPIValueWrapper::m_value gets visited, and add a
visitChildren() there for the !CPU(ADDRESS64) case.  Should probably do that in
a separate bug.

> Source/JavaScriptCore/heap/Heap.cpp:3011
> +void Heap::addmarkedJSValueRefArray(MarkedJSValueRefArray* array)

typo: /addmarkedJSValueRefArray/addMarkedJSValueRefArray/