Closed Bug 1058394 Opened 6 years ago Closed 6 years ago

Array#find and Array#findIndex no longer skip holes in ES6 draft rev 27

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: 446240525, Assigned: 446240525)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2135.0 Safari/537.36

Steps to reproduce:

Per https://bugs.ecmascript.org/show_bug.cgi?id=3107

[,1].find(()=>true)         // 1, should be undefined
[,1].findIndex(()=>true) // 1, should be 0
Attached patch bug1058394.diff (obsolete) — Splinter Review
Attached patch bug1058394 v2.patch (obsolete) — Splinter Review
Run "./mach mercurial-setup" then export a new patch.
Attachment #8478799 - Attachment is obsolete: true
Attachment #8479835 - Flags: review?(till)
Comment on attachment 8479835 [details] [diff] [review]
bug1058394 v2.patch

Review of attachment 8479835 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, thanks. Just a few comments on the comments (no pun intended), and a request for additional tests:

Please test that properties on the prototype are found, and that getters on the prototype that throw exceptions do throw as expected.

Also, please slighty change the description: "Don't skip holes in Array#find and Array#findIndex." That reflects what the change does, not *why* it does it (which is something that should be, and is, reflected in the bug).

::: js/src/builtin/Array.js
@@ +434,2 @@
>      /* Steps 8-9. */
>      /* Steps a (implicit), and e. */

Please change "e" to "g" in this comment.

@@ +439,5 @@
>       * Array.prototype.find.call(obj, () => true);
>       */
>      for (var k = 0; k < len; k++) {
> +        /* Step d. */
> +        var kValue = O[k];

The step comments are off here. The comment above this line should be "Steps a-c."

@@ +440,5 @@
>       */
>      for (var k = 0; k < len; k++) {
> +        /* Step d. */
> +        var kValue = O[k];
> +        if (callFunction(predicate, T, kValue, k, O))

And the one above this line should be "Steps d-f."

@@ +469,2 @@
>      /* Steps 8-9. */
>      /* Steps a (implicit), and e. */

Please change "e" to "g" in this comment.

@@ +473,5 @@
>       * var obj = { 18014398509481984: true, length: 18014398509481988 };
>       * Array.prototype.find.call(obj, () => true);
>       */
>      for (var k = 0; k < len; k++) {
> +        /* Step d. */

This should be "Steps a-f."
Attachment #8479835 - Flags: review?(till) → feedback+
Attachment #8479835 - Attachment is obsolete: true
Attachment #8480374 - Flags: review?(till)
Comment on attachment 8480374 [details] [diff] [review]
bug1058394 v3.patch

Review of attachment 8480374 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for the quick update.

I sent the patch to our try server, and will check it in once that returns green:
remote:   https://tbpl.mozilla.org/?tree=Try&rev=cedcba97a701
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cedcba97a701
Attachment #8480374 - Flags: review?(till) → review+
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(In reply to Till Schneidereit [:till] from comment #5)
> Comment on attachment 8480374 [details] [diff] [review]
> bug1058394 v3.patch
> 
> Review of attachment 8480374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks for the quick update.
> 
> I sent the patch to our try server, and will check it in once that returns
> green:
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=cedcba97a701
> remote: Alternatively, view them on Treeherder (experimental):
> remote:  
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cedcba97a701

Thanks till! This will be my first accepted patch for SpiderMonkey, I'm super excited now!
Updated polyfills for Array#find and Array#findIndex on MDN.
I landed the patch in our inbound tree:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0cba3e5a377d

That means it'll get merged to mozilla-central soon (probably in a few hours) and will most likely be part of the next Nightly.

> Thanks till! This will be my first accepted patch for SpiderMonkey, I'm
> super excited now!

Thanks again for the patch, it's very much appreciated.

I saw that you've already picked up other stuff, but if you're interested in ES6 compatibility stuff, you could take a look at the blockers of our ES6 meta bug:
https://bugzilla.mozilla.org/showdependencytree.cgi?id=694100&hide_resolved=1

Most of these are fairly involved, but there are also smaller tasks that would be good next bugs to work on. Also feel free to drop by in the #jsapi channel on irc.mozilla.org if you have any questions.
Assignee: nobody → 446240525
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/0cba3e5a377d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.