Closed Bug 1111869 Opened 6 years ago Closed 6 years ago

Implement %TypedArray%.prototype.includes

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: d, Assigned: 446240525)

References

Details

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

Attachments

(1 file, 1 obsolete file)

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
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.
Blocks: es7
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(446240525)
Assignee: nobody → 446240525
Flags: needinfo?(446240525)
Blocks: 1070767
Attached patch bug-1111869-v1.patch (obsolete) — Splinter Review
Attachment #8539639 - Flags: review?(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
Attachment #8539639 - Flags: review?(till) → review+
Also fixed style nits in tests for other typed array methods.
Attachment #8539639 - Attachment is obsolete: true
Attachment #8541258 - Flags: review?(till)
Comment on attachment 8541258 [details] [diff] [review]
review comment addressed

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

Very nice, thank you!
Attachment #8541258 - Flags: review?(till) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12272cbd508a
OS: Windows 8.1 → All
Hardware: x86_64 → All
Whiteboard: [DocArea=JS]
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/c30c6cdaaa83
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.