575685 - implement set() from Typed Arrays spec

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

We're missing this (array-to-array copy, with type conversion).

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: implement set()

This should do it.

Comment 2

[Jason Orendorff [:jorendorff]]

I haven't read the patch closely, but the tests need some work. They don't cover most of the branches in the code. Things you could test, just offhand:

  - code that tries to ask js_malloc for huge amounts of memory
  - arg0 is larger than `this`
  - arg0 is a stupid Array, like Array(0x7fffffff)
  - index < this.length but index + arg0.length > this.length
  - the cases where the regions being copied actually overlap

If TypedArray has taught us anything it is that we can do the work now of testing these oddball cases, or we can do it later in small pieces, one crash at a time.

Separately: If I understand the intent here, the constraint should (aesthetically speaking) be:
    0 <= index <= index + arg0.length <= this.length
in which case arr.set([], arr.length) should be allowed.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 465834 [details] [diff] [review]
implement set()

Blanking r?jorendorff; ping me if this is not the right thing to do.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: implement set(), v2

Now with more tests!  I also changed the condition, because as you say, set([]) should be fair-game anywhere.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 466831 [details] [diff] [review]
implement set(), v2

In fun_set:
>+        Value *argv;
>+        JSObject *obj;
>+
>+        argv = JS_ARGV(cx, vp);
>+        obj = ComputeThisFromVp(cx, vp);

Combine the declarations and assignments please.

>+            if (!src ||
>+                src->length > tarray->length - offset)
>+            {

All this goes on one line, in js/src.

>+            // avoid overflow; we know that offset < length

<=, not <, right?

>+            if (len > tarray->length - offset) {
>+                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                     JSMSG_TYPED_ARRAY_BAD_ARGS);
>+                return false;
>+            }
>+
>+            ok = tarray->copyFrom(cx, arg0, len, offset);
>+        }
>+
>+        if (!ok) {
>+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                 JSMSG_TYPED_ARRAY_BAD_ARGS);
>+            return false;
>+        }
>+
>+        vp->setUndefined();
>+        return ok;
>+    }

If copyFrom returns false, an error has already been reported, so the code can
just do:
    if (!tarray->copyFrom(blah blah))
        return false;
in both branches, the "if (!ok)" block can just be an else block, and the "ok"
variable can be deleted.

copyFromWithOverlap can't return false; make its return type void.

In copyFromWithOverlap(TypedArray:
>+          case TypedArray::TYPE_UINT16: {
>+            uint16 *src = (uint16*) js_malloc(tarray->byteLength);
>+            memcpy(src, tarray->data, tarray->byteLength);
>+            for (uintN i = 0; i < srclen; ++i)
>+                *dest++ = NativeType(*src++);
>+            js_free(src);
>+            break;
>+          }

Factor out the js_malloc and memcpy calls in front of the switch statement (use
a `void *srcbuf`; each case still needs a cast), and the js_free(src) after.

In js/src/tests/js1_8_5/extensions/typedarray.js:
>+    a.set([1,2,3,4,5,6,7,8,9]);
>+    a.set(a.slice(0,6), 3);
>+    check(function()
>+          a[0] == 1 && a[1] == 2 && a[2] == 3 &&
>+          a[3] == 1 && a[4] == 2 && a[5] == 3 &&
>+          a[6] == 4 && a[7] == 5 && a[8] == 6);

Thanks for the tests. Please add one more check after this, for the other kind
of overlap:

a.set(a.slice(3, 9), 0);
check(function()
      a[0] == 1 && a[1] == 2 && a[2] == 3 &&
      a[3] == 4 && a[4] == 5 && a[5] == 6 &&
      a[6] == 4 && a[7] == 5 && a[8] == 6);

r=me with the nits picked.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: patch for checkin v2.1

nits picked; attaching for posterity

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/tracemonkey/rev/82ff7c27fcb0

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/82ff7c27fcb0

Comment 9

[Eric Shepherd [:sheppy]]

Documented on all Typed Arrays types.