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

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: evilpie, Assigned: 446240525, Mentored)

Tracking

({dev-doc-complete})

Trunk
mozilla37
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

5 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

5 years ago
Mentor: evilpies

Comment 1

5 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

5 years ago
Assignee: nobody → 446240525
Assignee

Comment 3

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

Comment 4

5 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

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

Comment 7

5 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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.