Bug 204214 – [JSC] Anonymous built-in functions should have empty string for a name

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ross Kirsling]

[JSC] Anonymous built-in functions should have empty string for a name

[Ross Kirsling]

Created attachment 383590 [details]
Patch

[Ross Kirsling]

Note: These fixes are associated with https://github.com/tc39/ecma262/pull/1490.

[EWS Watchlist]

Comment on attachment 383590 [details]
Patch

Attachment 383590 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13257634

New failing tests:
imported/blink/fast/events/panScroll-crash.html

[EWS Watchlist]

Created attachment 383601 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ross Kirsling]

Created attachment 383643 [details]
Patch

[Ross Kirsling]

Created attachment 383648 [details]
Patch

[Ross Kirsling]

Created attachment 383649 [details]
Patch

[Ross Kirsling]

Created attachment 383663 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 383663 [details]
Patch

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

r=me with nits.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:244
> -            function @resolve(resolution) {
> +            function (resolution) {
>                  return @resolvePromiseWithFirstResolvingFunctionCallCheck(capturedPromise, resolution);
>              },
> -            function @reject(reason) {
> +            function (reason) {
>                  return @rejectPromiseWithFirstResolvingFunctionCallCheck(capturedPromise, reason);

You need to change InternalPromise constructor's one too.

> Source/JavaScriptCore/runtime/ProxyRevoke.cpp:54
> +    Base::finishCreation(vm, ""_s);

Pass `emptyString()`.

[Ross Kirsling]

Created attachment 383678 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383678 [details]
Patch for landing

Clearing flags on attachment: 383678

Committed r252520: <https://trac.webkit.org/changeset/252520>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57248654>

[Darin Adler]

Comment on attachment 383678 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/FunctionRareData.h:129
> +    FunctionRareData(VM&);

In a case like this it’s good to add "explicit" since this is now a single-argument constructor and "explicit" has value to make sure those aren’t accidentally used for type conversion.

On the other hand, with modern C++ we could be using explicit a lot more on most constructors that aren’t intended to be used for type conversion, even ones that don’t have a single argument, and that would affect a lot of code and not sure everyone agrees this would be good.

[Darin Adler]

Comment on attachment 383678 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:109
> +    return m_rareData ? m_rareData->hasReifiedName() : false;

I personally like the && syntax for cases like this better than ? : like this and find that it is easy to understand and reason about:

    return m_rareData && m_rareData->hasReifiedName();

[Ross Kirsling]

(In reply to Darin Adler from comment #14)
> Comment on attachment 383678 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383678&action=review
> 
> > Source/JavaScriptCore/runtime/FunctionRareData.h:129
> > +    FunctionRareData(VM&);
> 
> In a case like this it’s good to add "explicit" since this is now a
> single-argument constructor and "explicit" has value to make sure those
> aren’t accidentally used for type conversion.

Good point! Will address.


(In reply to Darin Adler from comment #15)
> Comment on attachment 383678 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383678&action=review
> 
> > Source/JavaScriptCore/runtime/JSFunctionInlines.h:109
> > +    return m_rareData ? m_rareData->hasReifiedName() : false;
> 
> I personally like the && syntax for cases like this better than ? : like
> this and find that it is easy to understand and reason about:
> 
>     return m_rareData && m_rareData->hasReifiedName();

I guess I was thinking in JavaScript mode where this wouldn't return the right type, but I guess the coercion works to one's advantage in C++, doesn't it. :P

[Ross Kirsling]

Committed r252547: <https://trac.webkit.org/changeset/252547>

[Darin Adler]

(In reply to Ross Kirsling from comment #16)
> (In reply to Darin Adler from comment #14)
> > I personally like the && syntax for cases like this better than ? : like
> > this and find that it is easy to understand and reason about:
> > 
> >     return m_rareData && m_rareData->hasReifiedName();
> 
> I guess I was thinking in JavaScript mode where this wouldn't return the
> right type, but I guess the coercion works to one's advantage in C++,
> doesn't it. :P

If the "coercion" wasn't present in C++ I would probably write:

    return m_rareData != nullptr && m_rareData->hasReifiedName();