[webkit-reviews] review granted: [Bug 214294] Remove ArrayNode::m_optional : [Attachment 404211] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 214294: Remove ArrayNode::m_optional
https://bugs.webkit.org/show_bug.cgi?id=214294

Attachment 404211: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 404211
  --> https://bugs.webkit.org/attachment.cgi?id=404211
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +	    `false` for empty array literals. 
> 
> Can you explain what the intention of m_optional was in the first place, and
why it is ok to remove it?

m_optional dates back to before the WebKit project began. It was already in KJS
when we started the WebKit project, although back then it was named "opt":

https://trac.webkit.org/browser/webkit/trunk/JavaScriptCore/kjs/nodes.h?rev=6

During that early stage KJS reflected many misunderstandings of the JavaScript
language specification that we cleared up over time.

If you look carefully you can see that the opt boolean only ever meant "is this
an array with elisions, an empty array, or one with a trailing comma". That
wasn’t an important distinction. I don’t think this ever had a valuable
function.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:509
> -    if (m_elision || m_optional)
> +    if (m_elision)

This is the only behavior change, so this is where we should focus. This is
obviously correct. Nothing about an empty array or one with a trailing comma
makes it different, or necessary to return false to this predicate.