1111869 - Implement %TypedArray%.prototype.includes

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Domenic Denicola]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045

Steps to reproduce:

In bug #1069063, Array.prototype.includes was implemented. As briefly confirmed at [1], there should be a corresponding method on %TypedArray%. The spec [2] has been updated; it is, like most of the typed array duplicate methods, pretty straightforward.

Tests are available at [3].

[1]: https://esdiscuss.org/topic/typedarray-prototype-includes
[2]: https://github.com/tc39/Array.prototype.includes/blob/master/spec.md#typedarrayprototypeincludes--searchelement---fromindex--
[3]: https://github.com/tc39/Array.prototype.includes/tree/master/test/built-ins/TypedArray/prototype/includes

Comment 1

[Till Schneidereit [:till]]

Thanks for filing this. Ziyunfei, would you by any chance be interested in taking this, too? Let's keep it behind the same compile flag as the Array.prototype version for now.

Comment 2

[ziyunfei]

Attachment: bug-1111869-v1.patch

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8539639 [details] [diff] [review]
bug-1111869-v1.patch

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

Most excellent, thanks!

::: js/src/builtin/TypedArray.js
@@ +197,5 @@
>      return O;
>  }
> +
> +// Proposed for ES7:
> +// https://github.com/domenic/Array.prototype.includes/blob/master/spec.md

I just realize that it'd be good to include the specific revision this is based on in the link. Also, the spec moved to the tc39 repoository:
https://github.com/tc39/Array.prototype.includes/blob/7c023c19a0/spec.md#typedarrayprototypeincludes--searchelement---fromindex--

r=me for making this change for the Array version, too. No extra patch needed, just do it in this one.

::: js/src/tests/ecma_6/TypedArray/includes.js
@@ +10,5 @@
> +    Float64Array
> +];
> +
> +for (var constructor of constructors) {
> +    if ("includes" in constructor.prototype) {

It's not all that great that we do this for every iteration. Then again, it doesn't do any harm, really.
However, you could change it to this to bail out early and remove one level of indentation from the loop body:
if (!("includes" in constructor.prototype))
    break;

@@ +26,5 @@
> +        assertEq(new constructor([1, 2, 3]).includes(2, 2), false);
> +        assertEq(new constructor([1, 2, 3]).includes(2, -1), false);
> +        assertEq(new constructor([1, 2, 3]).includes(2, 100), false);
> +
> +        // called from other globals

nit: start full-sentence comments with upper-case letter and end with ".". Here and below

@@ +33,5 @@
> +            assertEq(includes.call(new constructor([1, 2, 3]), 2), true);
> +        }
> +
> +        // throws if `this` isn't a TypedArray
> +        var nonTypedArrays = [undefined, null, 1, false, "", Symbol(), [], {}, /./,

nit: I would call these something like "invalidReceivers": "nonTypedArrays" sounds like "untypedArrays" to me. Totally not important, though

@@ +34,5 @@
> +        }
> +
> +        // throws if `this` isn't a TypedArray
> +        var nonTypedArrays = [undefined, null, 1, false, "", Symbol(), [], {}, /./,
> +                             /* new Proxy(new constructor(), {}) // this probably should throw */

Do we have a bug for this on file? If not, please file one. In any case, change this to throw if we change to the correct behavior and add a comment linking to the bug. That way, we can be sure to have full test coverage here as soon as that's possible.

@@ +37,5 @@
> +        var nonTypedArrays = [undefined, null, 1, false, "", Symbol(), [], {}, /./,
> +                             /* new Proxy(new constructor(), {}) // this probably should throw */
> +                             ];
> +        nonTypedArrays.forEach(nonTypedArray => {
> +            assertThrowsInstanceOf(function() {

nit: you can use an arrow function here, too. Not important, though, so up to you

Comment 4

[ziyunfei]

Attachment: review comment addressed

Also fixed style nits in tests for other typed array methods.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8541258 [details] [diff] [review]
review comment addressed

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

Very nice, thank you!

Comment 6

[ziyunfei]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12272cbd508a

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c30c6cdaaa83

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c30c6cdaaa83

Comment 9

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/includes