[webkit-reviews] review requested: [Bug 206431] [JSC] Add support for private class fields : [Attachment 388545] Patch v7

[bugzilla-daemon at webkit.org]

Caitlin Potter (:caitp) <caitp at igalia.com> has asked  for review:
Bug 206431: [JSC] Add support for private class fields
https://bugs.webkit.org/show_bug.cgi?id=206431

Attachment 388545: Patch v7

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




--- Comment #11 from Caitlin Potter (:caitp) <caitp at igalia.com> ---
Comment on attachment 388545
  --> https://bugs.webkit.org/attachment.cgi?id=388545
Patch v7

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

I've tried to address each of these.

>> Source/JavaScriptCore/ChangeLog:24
>> +	    To illustrate, typical private field access will look like:
> 
> It would be nice have an explanation why we need to have this private symbol
lookup on CL, otherwise we could simply use any `get_by_id` variant. It could
be a short text explaining that different evaluations of class can't access
private fields of each other.

```
Private field names are loaded from scope before their use. This prevents
multiple evaluations
of the same class source from accessing each other's private fields, because
the values of the
symbols loaded from the class scope would be distinct. This is required by the
proposal text,
and is the key reason why we use ByVal lookups rather than ById lookups.
```

hope that covers it ��

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:868
>> +	    generator.emitProfileType(privateName.get(), var, m_position,
JSTextPosition(-1, m_position.offset, m_ident.length()));
> 
> I don't understand why we need `emitProfileType` here. I'm having a hard time
to see how this would be reported to the user, and how this would be relevant.

Based on the comments from part1, it's not. I guess there were more instances
of this added in part 2. Cleaning them up now.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:896
>> +	    generator.emitProfileType(privateName.get(), var, m_position,
JSTextPosition(-1, m_position.offset, m_ident.length()));
> 
> ditto.

gone

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2032
>> +	    generator.emitProfileType(privateName.get(), var, m_position,
JSTextPosition(-1, m_position.offset + ident.length(), -1));
> 
> ditto.

done

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4441
>> +	    generator.emitProfileType(privateName.get(), var, m_position,
JSTextPosition(-1, m_position.offset + m_ident->length(), -1));
> 
> ditto.

gone

>> Source/JavaScriptCore/jit/JITOperations.cpp:865
>> +	    if (found == (bytecode.m_flags & PutByValThrowIfExists)) {
> 
> Windows warns here:
> 
> JavaScriptCore\jit/JITOperations.cpp(865,64): warning C4805: '==': unsafe mix
of type 'bool' and type 'int' in operation
> 
> Could fix by adding !! prefix or != 0 suffix to the second half of the
expression.

Done

>> Source/JavaScriptCore/parser/Parser.h:488
>> +	void copyPrivateNameUses(Scope& other)
> 
> See comments on `VariableEnvironment.h`

Done

>> Source/JavaScriptCore/parser/Parser.h:1238
>> +	bool copyPrivateNameUsesToOuterScope()
> 
> I think it is better name this as
`copyUndeclaredPrivateNameUsesToOuterScope`.

Fair enough, done

>> Source/JavaScriptCore/parser/Parser.h:1249
>> +	    current->copyPrivateNameUses(m_scopeStack[i]);
> 
> See comments on `VariableEnvironment.h`

Done

>> Source/JavaScriptCore/parser/Parser.h:2034
>> +	if (isEvalNode<ParsedNode>() && variablesUnderTDZ &&
variablesUnderTDZ->privateNamesSize()) {
> 
> Maybe it is a good time to rename `variablesUnderTDZ` to
`privateNamesAndVariablesUnderTDZ`?

I'm not sure if the extra verbosity really adds much, especially since the
private names are really just variables anyways. If everyone else feels it's
worth it, then by all means.

>> Source/JavaScriptCore/parser/VariableEnvironment.h:223
>> +	ALWAYS_INLINE void copyPrivateNameUses(VariableEnvironment& outer)
const {
> 
> I think this would be better to understand if called
`copyUndeclaredPrivateNames`, given we don't copy declared private names.

fair enough, done

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1168
>> +	    });
> 
> nit: fix spacing.

I know check-webkit-style complains, but this is consistent with the style of
the rest of the file, so I don't think changing it is a good idea.