Bug 204214 - [JSC] Anonymous built-in functions should have empty string for a name
Summary: [JSC] Anonymous built-in functions should have empty string for a name
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-14 17:07 PST by Ross Kirsling
Modified: 2020-06-17 15:16 PDT (History)
11 users (show)

See Also:


Attachments
Patch (70.86 KB, patch)
2019-11-14 17:29 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (14.21 MB, application/zip)
2019-11-14 22:36 PST, EWS Watchlist
no flags Details
Patch (70.30 KB, patch)
2019-11-15 13:51 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (70.33 KB, patch)
2019-11-15 13:57 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (70.31 KB, patch)
2019-11-15 14:01 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (77.73 KB, patch)
2019-11-15 16:33 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (78.25 KB, patch)
2019-11-15 18:58 PST, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2019-11-14 17:07:26 PST
[JSC] Anonymous built-in functions should have empty string for a name
Comment 1 Ross Kirsling 2019-11-14 17:29:09 PST
Created attachment 383590 [details]
Patch
Comment 2 Ross Kirsling 2019-11-14 22:02:00 PST
Note: These fixes are associated with https://github.com/tc39/ecma262/pull/1490.
Comment 3 EWS Watchlist 2019-11-14 22:36:13 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-11-14 22:36:16 PST Comment hidden (obsolete)
Comment 5 Ross Kirsling 2019-11-15 13:51:33 PST
Created attachment 383643 [details]
Patch
Comment 6 Ross Kirsling 2019-11-15 13:57:12 PST
Created attachment 383648 [details]
Patch
Comment 7 Ross Kirsling 2019-11-15 14:01:19 PST
Created attachment 383649 [details]
Patch
Comment 8 Ross Kirsling 2019-11-15 16:33:01 PST
Created attachment 383663 [details]
Patch
Comment 9 Yusuke Suzuki 2019-11-15 18:06:12 PST
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()`.
Comment 10 Ross Kirsling 2019-11-15 18:58:24 PST
Created attachment 383678 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2019-11-15 19:44:34 PST
Comment on attachment 383678 [details]
Patch for landing

Clearing flags on attachment: 383678

Committed r252520: <https://trac.webkit.org/changeset/252520>
Comment 12 WebKit Commit Bot 2019-11-15 19:44:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-11-15 19:45:24 PST
<rdar://problem/57248654>
Comment 14 Darin Adler 2019-11-18 09:23:06 PST
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.
Comment 15 Darin Adler 2019-11-18 09:24:06 PST
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();
Comment 16 Ross Kirsling 2019-11-18 09:49:59 PST
(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
Comment 17 Ross Kirsling 2019-11-18 10:01:25 PST
Committed r252547: <https://trac.webkit.org/changeset/252547>
Comment 18 Darin Adler 2019-11-18 17:04:42 PST
(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();