[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 #9 from Xan Lopez <xan.lopez at gmail.com> ---
Hi,

thanks for the review.

(In reply to Saam Barati from comment #8)
> Comment on attachment 404819 [details]
> Add support for static public fields, v5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404819&action=review
> 
> > Source/JavaScriptCore/ChangeLog:43
> > +        following order: b, c, a, d. Since each group of fields is
> > +        initialized independently we can trivially retrieve the property
> > +        keys for each group just by getting the odd property keys for
> > +        static fields (indexes 1 and 3) and the even ones for instance
> > +        fields (indexes 0 and 2).
> 
> I don't get this. Why do we need property keys of numbers ever? Are we
> looking up in an array or something?

Most questions are about this in one way or another, so I'll try to answer them all here.

Computed properties need to be evaluated (and transformed into property keys) during the first class evaluation. *Instance* class fields are initialized later. That means we need some place to store the property keys meanwhile. In the already landed instance class fields patch (https://bugs.webkit.org/show_bug.cgi?id=174212) those property keys were stored in the class scope and could be retrieved through numerical indexes, essentially a poor man's array.

What this patch does is build up on that, splitting the property keys for static and instance fields in even/odd indexes because they need to be read separately. Again, this is a poor man's way of doing two arrays. Also, strictly speaking, we could do without this for static fields, but since we are also using a field initializer it's much easier to reuse the whole infrastructure in place.

At the very least all this could be changed to use proper arrays, I think I mentioned that possibility in an earlier comment. Maybe we could first improve the way this is done for instance fields, and when we are happy with it change this patch to use the same mechanism. Or something else! Open to any suggestion here, hopefully it's easier to iterate the right solution now that this patch passes all tests.

> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:823
> > +    RefPtr<RegisterID> propertyExpr = generator.emitNode(node.m_expression);
> > +    RefPtr<RegisterID> propertyName = generator.emitToPropertyKey(generator.newTemporary(), propertyExpr.get());
> > +
> > +    Ref<Label> validPropertyNameLabel = generator.newLabel();
> > +    RefPtr<RegisterID> prototypeString = generator.emitLoad(nullptr, JSValue(generator.addStringConstant(generator.propertyNames().prototype)));
> > +    generator.emitJumpIfFalse(generator.emitBinaryOp<OpEq>(generator.newTemporary(), prototypeString.get(), propertyName.get(), OperandTypes(ResultType::stringType(), ResultType::stringType())), validPropertyNameLabel.get());
> > +    generator.emitThrowTypeError("Cannot declare a static field named 'prototype'");
> >  
> > +    generator.emitLabel(validPropertyNameLabel.get());
> 
> this code in emitSaveComputedFieldName is only called for static fields?
> 

Good catch, the code used to be like that but this is a mistake, should be guarded for static fields only.

-- 
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/20200728/e49bb649/attachment.htm>