[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

Caio Lima <ticaiolima at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ticaiolima at gmail.com

--- Comment #3 from Caio Lima <ticaiolima at gmail.com> ---
Comment on attachment 400054
  --> https://bugs.webkit.org/attachment.cgi?id=400054
Add support for static public fields

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

I think it is necesssary to have some stress test cases included in this patch, since most of our bots don't run Test262. It is also important to have a test case asserting that the case listed on https://bugs.webkit.org/show_bug.cgi?id=209675 can pass.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:796
> +    generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary(), generator.emitLoad(nullptr, JSValue(generator.addStringConstant(generator.propertyNames().prototype))), propertyName, OperandTypes(ResultType::stringType(), ResultType::stringType())), validPropertyNameLabel.get());

I personally think this is very hard to read. Also, I'm not 100% sure if this quite safe if we don't use `RefPtr<>`. Do you mind making it something like:

```
RefPtr<RegisterID> prototypeString = generator.emitLoad(nullptr, generator.propertyNames().prototype);
RefPtr<RegisterID> isValidPropertyName = generator.emitBinaryOp<OpEq>(generator.newTemporary(), prototypeString, propertyName, OperandTypes(ResultType::stringType(), ResultType::stringType()))
generator.emitJumpIfFalse(isValidPropertyName, validPropertyNameLabel.get());
```

> Source/JavaScriptCore/parser/ASTBuilder.h:408
> +    DefineFieldNode* createDefineField(const JSTokenLocation& location, const Identifier* ident, ExpressionNode* initializer, DefineFieldNode::Type type, bool isStatic)

Could you turn `bool isStatic` into an enum?

> Source/JavaScriptCore/parser/Nodes.h:2295
> +        DefineFieldNode(const JSTokenLocation&, const Identifier*, ExpressionNode*, Type, bool);

Ditto

> Source/JavaScriptCore/parser/Parser.cpp:259
>          else if (parseMode == SourceParseMode::InstanceFieldInitializerMode) {

This is not accurate anymore, since it is also being used for static fields. I think I would prefer create a new `SourceParseMode::StaticFieldInitializerMode` with reason described bellow.

> Source/JavaScriptCore/parser/Parser.cpp:2872
> +    unsigned numInstanceComputedFields = 0, numStaticComputedFields = 1;

I think that a comment here to explain why `numStaticComputedFields` starts with 1 would help a lot. Also, WDYT if we call them as `nextInstanceComputedFieldID` and `nextStaticComputedFieldID`?

> Source/JavaScriptCore/parser/Parser.cpp:2978
> +                unsigned* fieldID = tag == ClassElementTag::Instance ? &numInstanceComputedFields : &numStaticComputedFields;

I'm also in favor of using:

```
if (tag == ClassElementTag::Instance)
    ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm, nextInstanceComputedFieldID++);
else
    ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm, nextStaticComputedFieldID++);
```

> Source/JavaScriptCore/parser/Parser.cpp:3047
> +template <class TreeBuilder> TreeSourceElements Parser<LexerType>::parseInstanceFieldInitializerSourceElements(TreeBuilder& context, const Vector<JSTextPosition>& classFieldLocations)

This function is also being used to initialize static fields. I think we should rename it to `parseFieldInitializerSourceElements`.

> Source/JavaScriptCore/parser/Parser.cpp:3053
> +    // fields, at 1 for static fields. We'll bump the counter by one

Instead of having this check down there, we could use the `parseMode` to properly initialize `numComputedFields`. I would also rename it as `nextComputedFieldID`.

> Source/JavaScriptCore/runtime/JSFunction.cpp:-759
> -    ASSERT(jsExecutable()->ecmaName().isNull());

Is there a reason to remove this ASSERT?

-- 
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/20200525/610902d8/attachment-0001.htm>