Implement %TypedArray%.prototype.{indexOf, lastIndexOf}

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: evilpie, Assigned: 446240525, Mentored)

Tracking

({dev-doc-complete})

Trunk
mozilla37
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
This is specified at:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototype.lastindexof
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototype.indexof
You can look at Bug #1078975 to get a general idea how this works.

So what you have to do is:
- Take a look at Array.js and copy the code for indexof and lastindexof to TypedArray.js
- Add the code for "This function is not generic" like it's done in TypedArrayFind.
- Add a reference to the functions in TypedArray.cpp in the TypedArrayObject::protoFunctions array. You can take "find" as a reference again.
- Add the function to the array in test_xrayToJS.xul, like it's done for Bug 1078975.
- Write some tests
(Reporter)

Updated

4 years ago
Mentor: evilpies

Comment 1

4 years ago
i would love to work on this!
(In reply to Hubert B Manilla from comment #1)
> i would love to work on this!

Excellent, please jump right in!

If you have any questions, ask here or join us in the #jsapi channel on irc.mozilla.org.
(Assignee)

Updated

4 years ago
Assignee: nobody → 446240525
(Assignee)

Comment 3

4 years ago
Posted patch bug-1107601-v1.patch (obsolete) — Splinter Review
Attachment #8535999 - Flags: review?(evilpies)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8535999 [details] [diff] [review]
bug-1107601-v1.patch

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

::: js/src/builtin/TypedArray.js
@@ +92,5 @@
> +    if (len === 0)
> +        return -1;
> +
> +    // Steps 7-8.
> +    var n =  ToInteger(fromIndex);

One space to many.

::: js/src/tests/ecma_6/TypedArray/indexOf_lastIndexOf.js
@@ +1,1 @@
> +const constructors = [

Please rename this file to indexOf-and-lastIndexOf.js.

I really like your tests \o/

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

Mhm yeah interesting. Reminds me of bug 1096753, but ecma apparently decided that this is okay at least for Array.isArray. We should probably at least have a bug for this.

@@ +40,5 @@
> +        }, TypeError, "Assert that indexOf fails if this value is not a TypedArray");
> +    });
> +
> +    // test that this.length is never called
> +    assertEq(Object.defineProperty(new Int8Array([0, 1, 2, 3, 5]), "length", {

new constructor

@@ +83,5 @@
> +        get() {
> +            throw new Error("length accessor called");
> +        }
> +    }).lastIndexOf(1), 1);
> +}

I would like to see some tests for Float32Array/Float64Array that look for different floats. Especially correct NaN handling.
Attachment #8535999 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #4)
> > +    // throws if `this` isn't a TypedArray
> > +    var nonTypedArrays = [undefined, null, 1, false, "", Symbol(), [], {}, /./,
> > +                         /* new Proxy(new constructor(), {}) // this should throw */
> 
> Mhm yeah interesting. Reminds me of bug 1096753, but ecma apparently decided
> that this is okay at least for Array.isArray. We should probably at least
> have a bug for this.

I think the situation is somewhat different for typed arrays, maybe. They give the developer certain guarantees such as "assigning (a value of the right type) to an index within the range cannot throw", same for accessing indices. Crucially, in this case we're the developer, and we make use of just those guarantees in the implementation. If we let proxies through here, we lose them.

I'd argue that people who want to have generic list methods apply to all of their instances, no matter what their implementation strategy might be, should just use the Array methods.
(Assignee)

Comment 6

4 years ago
Attachment #8535999 - Attachment is obsolete: true
Attachment #8536019 - Flags: review?(evilpies)
(Reporter)

Comment 7

4 years ago
Comment on attachment 8536019 [details] [diff] [review]
review comment addressed

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

Thank you! You already have quite a lot of experience, so please open your own bugs (e.g. there are still a lot of TypedArray methods without a bug) instead of using mentored bugs in the future.
Attachment #8536019 - Flags: review?(evilpies) → review+
https://hg.mozilla.org/mozilla-central/rev/d70b753290cc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.