[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

--- Comment #5 from Xan Lopez <xan.lopez at gmail.com> ---
Addressed most of these, commenting inline.

Additional comments:

- There are three failures in the new V8 tests, will look into them now.
- As we discussed I think it would be better to store the computed field keys in something like a list. Will also try to look into this.

(In reply to Caio Lima from comment #3)
> Comment on attachment 400054 [details]
> 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.

I imported the V8 tests, like I did with the instance fields. Also refactored a bit both files so they can share the support code.

> 
> > 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());
> ```

Done, although the exact code you suggested does not work as we discussed offline. Will look into this.

> 
> > 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) {

Turns out this was not needed, because the parser code just reparses the fields and figures out which 'mode' it's in (static or instance). We could tell it explicitly, but since it needs to reparse the fields anyway it seemed redundant.

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

All done.

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

This was a leftover from a previous way of doing things! Removed.

-- 
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/20200605/56ae2b8d/attachment.htm>