Implement %TypedArray%.prototype.{keys, values, entries}

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

({dev-doc-complete})

Trunk
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

We already have %TypedArray%.prototype.[@@iterator], but we need to adjust it after implementing values:
"The initial value of the @@iterator property is the same function object as the initial value of the %TypedArray%.prototype.values property." (22.2.3.31)
Assignee: nobody → evilpies
Comment on attachment 8547084 [details] [diff] [review]
v1 - Implement %TypedArray%.prototype.{keys, values, entries}

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

Most excellent. r=me with nits addressed.

::: js/src/builtin/TypedArray.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// ES6 draft rev29 (2014/12/06) 22.2.3.6 %TypedArray%.prototype.entries()

New draft is out (and even referenced one function down), so please update the reference here and below.

@@ +11,5 @@
> +    if (!IsObject(O) || !IsTypedArray(O)) {
> +        return callFunction(CallTypedArrayMethodIfWrapped, O, "TypedArrayEntries");
> +    }
> +
> +    // Step 4-6. ToDo: detachment

Please link to a bug covering detachment for all methods here and below.

::: js/src/vm/CommonPropertyNames.h
@@ +210,5 @@
>      macro(useGrouping, useGrouping, "useGrouping") \
>      macro(useAsm, useAsm, "use asm") \
>      macro(useStrict, useStrict, "use strict") \
>      macro(value, value, "value") \
> +    macro(values, values, "values") \

nice

::: js/src/vm/TypedArrayObject.cpp
@@ +696,5 @@
>  
> +static bool
> +FinishTypedArrayInit(JSContext *cx, HandleObject ctor, HandleObject proto)
> +{
> +    // Define values and @@iterator manually, because they are supposed to be the same object.

Use "`" or "|" around "values" and "@@iterator" to denote them as code.

@@ +706,5 @@
> +    RootedValue funValue(cx, ObjectValue(*fun));
> +    if (!JSObject::defineProperty(cx, proto, cx->names().values, funValue))
> +        return false;
> +
> +#ifdef JS_HAS_SYMBOLS

Do we even do symbol-less builds anymore? We should probably rip out the ifdefs soon-ish.

@@ +802,5 @@
>  }
>  
>  /* static */ const JSFunctionSpec
>  TypedArrayObject::protoFunctions[] = {
> +    JS_SELF_HOSTED_SYM_FN(iterator, "TypedArrayValues", 0, JSPROP_DEFINE_LATE),

I would move this to the bottom so it comes right after "values".

@@ +819,5 @@
>      JS_SELF_HOSTED_FN("reverse", "TypedArrayReverse", 0, 0),
>      JS_SELF_HOSTED_FN("some", "TypedArraySome", 2, 0),
> +    JS_SELF_HOSTED_FN("entries", "TypedArrayEntries", 0, 0),
> +    JS_SELF_HOSTED_FN("keys", "TypedArrayKeys", 0, 0),
> +    JS_SELF_HOSTED_FN("values", "TypedArrayValues", 0, JSPROP_DEFINE_LATE),

So this essentially has documentation value (no pun intended), right? That should at least be pointed out with a comment referring to the function to `FinishTypedArrayInit`.

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +178,5 @@
>    }
>    gPrototypeProperties['TypedArray'] =
>      ["length", "buffer", "byteLength", "byteOffset", kIteratorSymbol, "subarray",
>       "set", "copyWithin", "find", "findIndex", "indexOf", "lastIndexOf", "reverse",
> +     "join", "every", "some", "reduce", "reduceRight", "entries", "keys", "values"];

An xpconnect peer needs to r+ this. (Though I wonder how useful that is, really. What's a scenario where we'd get an r- for implementing a spec'ed method?)
Attachment #8547084 - Flags: review?(till) → review+
Can you give a comment if adding those functions to test_xrayToJS.xul is fine? I actually allowed some other patches before that just added functions here as well.
Flags: needinfo?(bzbarsky)
Yes, that looks fine.  The idea is that the things that should show up there are whatever would appear on vanilla TypedArray prototype objects that no one has messed with, so as we add new stuff to typed arrays they will need to be added to that test.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/93c6ac70dc60
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.