[Webkit-unassigned] [Bug 186694] [ESNExt] String.prototype.matchAll

[bugzilla-daemon at webkit.org]

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

--- Comment #31 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 372055
  --> https://bugs.webkit.org/attachment.cgi?id=372055
Patch

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

>>>> Source/JavaScriptCore/builtins/RegExpPrototype.js:28
>>>> +function RegExpStringIteratorConstructor(R, S, global, fullUnicode)
>>> 
>>> Rename this to createRegExpStringIterator.
>> 
>> Using `new` with `create*` function looks kinda weird.
>> Idiomatic JS is `new` with title-cased constructor, just like we already have with AsyncFromSyncIterator:
>> 
>> ```
>> return new @AsyncFromSyncIteratorConstructor(syncIterator, nextMethod);
>> ```
>> 
>> AsyncFromSyncIterator does indeed expose `createAsyncFromSyncIterator` helper for BytecodeGenerator,
>> but I not sure it is something we need for RegExpStringIterator.
>> 
>> ===
>> 
>> On performance: we can't use any of already defined fast RegExp methods.
>> We can avoid extra sets and property lookups in two places:
>> 1. After @speciesConstructor in RegExp.prototype[@@matchAll]
>> 2. After last @getByIdDirectPrivate in %RegExpStringIteratorPrototype%.next.
>> 
>> Given that we will have to call @hasObservableSideEffects in `next` every time, does it worth it?
> 
> But instead, you introduced "RegExpStringIteratorConstructor" variable in JSGlobalObject.cpp. In WebKit, variables start with lower case.
> If you want to use this, can you do the following things too?
> 
> 1. Rename RegExpStringIteratorConstructor to RegExpStringIterator
> 2. Fix the script not to expose RegExpStringIterator as a variable. Instead, we should expose regExpStringIterator
> 3. And fix all the existing ones too.
> 
> For performance perspective, I think this is OK for the first implementation.

Hmm. After considering about my proposal above, I think above proposal is not perfect yet :(
I think the better one should be the followings.

1. Function name in JS should be RegExpStringIterator. Remove "Constructor".
2. But the variable name should have "Constructor" in C++
3. We do not use title-case variable name.

So, I think the better fix is,

1. If @constructor annotation is attached, we only allowed title-case name in builtin JS function (ensuring it in the builtin script processor). I think we can also assert that all non @constructor function starts with lower-case character.
2. If @constructor annotation is attached, we convert title-case name to usual WebKit variable name for C++ code, and add "Constructor" name too for C++ in the builtin script processor.
3. And apply the above changes to the all existing ones.

So, the code looks like,

@globalPrivate
@constructor
function RegExpStringIterator(......)
{
    ...
}

And, C++ code looks like,

regExpStringIteratorConstructor ...

I think this is better.

-- 
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/20190617/5d5d398d/attachment.html>