Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JSC] Rename Array#groupBy to Array#group and enable them #2990

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Aug 3, 2022

317e700

[JSC] Rename Array#groupBy to Array#group and enable them
https://bugs.webkit.org/show_bug.cgi?id=243510

Reviewed by Mark Lam, Justin Michaud, Devin Rousso and Ross Kirsling.

We agreed at TC39 to renaming Array#groupBy and Array#groupByToMap to Array#group and Array#groupToMap
to keep web-compatibility. This patch renames and enables them, stage-4 progress is tracked in [1].
We also upstreamed test262 fix for this[2].

[1]: tc39/proposal-array-grouping#32
[2]: tc39/test262#3632

* Source/JavaScriptCore/builtins/ArrayPrototype.js:
(group):
(groupToMap):
(groupBy): Deleted.
(groupByToMap): Deleted.
* Source/JavaScriptCore/runtime/ArrayPrototype.cpp:
(JSC::ArrayPrototype::finishCreation):
* Source/JavaScriptCore/runtime/OptionsList.h:

Canonical link: https://commits.webkit.org/253101@main

@Constellation Constellation requested a review from a team as a code owner August 3, 2022 20:17
@Constellation Constellation self-assigned this Aug 3, 2022
@Constellation Constellation added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Aug 3, 2022
Copy link
Member

@rkirsling rkirsling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@Constellation Constellation force-pushed the eng/JSC-Rename-ArraygroupBy-to-Arraygroup-and-enable-them branch from 6b2ba67 to 330465b Compare August 3, 2022 20:22
Copy link

@MenloDorian MenloDorian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me too

Copy link
Contributor

@justinmichaud justinmichaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with question

@@ -541,7 +541,7 @@ bool canUseWebAssemblyFastMemory();
/* Feature Flags */\
\
v(Bool, useArrayFindLastMethod, true, Normal, "Expose the findLast() and findLastIndex() methods on Array and %TypedArray%.") \
v(Bool, useArrayGroupByMethod, false, Normal, "Expose the groupBy() and groupByToMap() methods on Array.") \
v(Bool, useArrayGroupMethod, true, Normal, "Expose the group() and groupToMap() methods on Array.") \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We enable this by default?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this patch enables it.

@Constellation Constellation force-pushed the eng/JSC-Rename-ArraygroupBy-to-Arraygroup-and-enable-them branch from 330465b to a046d9f Compare August 3, 2022 20:25
Copy link
Member

@dcrousso dcrousso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also update the entries in WI.NativePrototypeFunctionParameters for Web Inspector? :)

@Constellation Constellation force-pushed the eng/JSC-Rename-ArraygroupBy-to-Arraygroup-and-enable-them branch from a046d9f to c555596 Compare August 3, 2022 22:35
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Aug 4, 2022
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Aug 4, 2022
@Constellation Constellation force-pushed the eng/JSC-Rename-ArraygroupBy-to-Arraygroup-and-enable-them branch from c555596 to 793f554 Compare August 4, 2022 03:59
@Constellation Constellation added the merge-queue Applied to send a pull request to merge-queue label Aug 4, 2022
https://bugs.webkit.org/show_bug.cgi?id=243510

Reviewed by Mark Lam, Justin Michaud, Devin Rousso and Ross Kirsling.

We agreed at TC39 to renaming Array#groupBy and Array#groupByToMap to Array#group and Array#groupToMap
to keep web-compatibility. This patch renames and enables them, stage-4 progress is tracked in [1].
We also upstreamed test262 fix for this[2].

[1]: tc39/proposal-array-grouping#32
[2]: tc39/test262#3632

* Source/JavaScriptCore/builtins/ArrayPrototype.js:
(group):
(groupToMap):
(groupBy): Deleted.
(groupByToMap): Deleted.
* Source/JavaScriptCore/runtime/ArrayPrototype.cpp:
(JSC::ArrayPrototype::finishCreation):
* Source/JavaScriptCore/runtime/OptionsList.h:

Canonical link: https://commits.webkit.org/253101@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/JSC-Rename-ArraygroupBy-to-Arraygroup-and-enable-them branch from 793f554 to 317e700 Compare August 4, 2022 05:03
@webkit-commit-queue
Copy link
Collaborator

Committed 253101@main (317e700): https://commits.webkit.org/253101@main

Reviewed commits have been landed. Closing PR #2990 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 317e700 into WebKit:main Aug 4, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
7 participants