[webkit-reviews] review denied: [Bug 194095] [JSC] Add support for static public class fields : [Attachment 414165] Add support for static public fields, v9

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Xan Lopez <xan.lopez at gmail.com>'s
request for review:
Bug 194095: [JSC] Add support for static public class fields
https://bugs.webkit.org/show_bug.cgi?id=194095

Attachment 414165: Add support for static public fields, v9

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




--- Comment #15 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 414165
  --> https://bugs.webkit.org/attachment.cgi?id=414165
Add support for static public fields, v9

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

Overall looks good. But I put r- since I have several questions about possible
bugs.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:827
> +	  
generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary()
, prototypeString.get(), propertyName.get(),
OperandTypes(ResultType::stringType(), ResultType::stringType())),
validPropertyNameLabel.get());

Use OpStrictEq.

> Source/JavaScriptCore/parser/Parser.cpp:2885
> +    unsigned nextInstanceComputedFieldID = 0, nextStaticComputedFieldID = 0;

Use
unsigned nextInstanceComputedFieldID = 0;
unsigned nextStaticComputedFieldID = 0;

we do not use comma typically here.

> Source/JavaScriptCore/parser/Parser.cpp:3080
> +    bool isStaticField = false;

Why is it defined outside of for loop? What happens if the first field is
static and next field is not static (I think in that case, isStaticField
boolean becomes wrong).
Can you add a test about this case?

> Source/JavaScriptCore/parser/Parser.cpp:3098
> +	   if (match(RESERVED_IF_STRICT) && *m_token.m_data.ident ==
m_vm.propertyNames->staticKeyword) {
> +	       ident = m_token.m_data.ident;
> +	       ASSERT(ident);
> +	       next();
> +	       if (match(SEMICOLON) || match (EQUAL) || match(CLOSEBRACE) ||
m_lexer->hasLineTerminatorBeforeToken())
> +		   goto validField;
> +	       isStaticField = true;
> +	   }

Let's set ident only when it is valid field, and check ident with nullptr. And
then, avoid using goto.
Like

if () {
    auto* staticIdentifier =  m_token.m_data.ident;
    ASSERT(staticIdent);
    next();
    if (match(SEMICOLON) || match (EQUAL) || match(CLOSEBRACE) ||
m_lexer->hasLineTerminatorBeforeToken())
	ident = staticIdentifier;
    else
	isStaticField = true;
}

if (!ident) {
    switch (...) {
   ...
}

> Source/JavaScriptCore/parser/Parser.cpp:3139
> +    validField:

Let's avoid using goto.