[Webkit-unassigned] [Bug 194095] [JSC] Add support for static public class fields

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=194095

Yusuke Suzuki <ysuzuki at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #414165|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20201117/7d8effb8/attachment-0001.htm>