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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

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

Comment 1

[ziyunfei]

Attachment: bug1058394.diff

Comment 2

[ziyunfei]

Attachment: bug1058394 v2.patch

Run "./mach mercurial-setup" then export a new patch.

Comment 3

[Till Schneidereit [: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."

Comment 4

[ziyunfei]

Attachment: bug1058394 v3.patch

Comment 5

[Till Schneidereit [: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

Comment 6

[ziyunfei]

(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!

Comment 7

[ziyunfei]

Updated polyfills for Array#find and Array#findIndex on MDN.

Comment 8

[Till Schneidereit [:till]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0cba3e5a377d