Last Comment Bug 795505 - Feeding TypedArray to js-ctypes
: Feeding TypedArray to js-ctypes
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on:
Blocks: 771094
  Show dependency treegraph
 
Reported: 2012-09-28 15:49 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-10-09 01:20 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Feeding typed arrays to js-ctypes (6.88 KB, patch)
2012-09-29 01:28 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Splinter Review
Feeding typed arrays to js-ctypes, v2 (9.81 KB, patch)
2012-10-03 02:54 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Splinter Review
Feeding typed arrays to js-ctypes, v3 (11.81 KB, patch)
2012-10-03 10:09 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Feeding typed arrays to js-ctypes, v3 (12.46 KB, patch)
2012-10-03 11:39 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Splinter Review
2. More tests (1.77 KB, patch)
2012-10-05 07:33 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Splinter Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-28 15:49:41 PDT
Bug 732936 lets users feed ArrayBuffer to js-ctypes.
We should do the same with TypedArray.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-28 23:22:37 PDT
After toying with the idea, I tend to believe that we should accept to convert any typed array to any array/pointer type, without attempting to add a veneer of type-safety.
Comment 2 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-29 01:28:45 PDT
Created attachment 666181 [details] [diff] [review]
Feeding typed arrays to js-ctypes
Comment 3 Jason Orendorff [:jorendorff] 2012-10-02 15:32:42 PDT
Comment on attachment 666181 [details] [diff] [review]
Feeding typed arrays to js-ctypes

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

Yep, looks correct. r=me with stricter implicit conversions.

::: js/src/ctypes/CTypes.cpp
@@ +2194,5 @@
> +
> +      // FIXME: Should we need to, we can obtain the size of an element in the
> +      // array as follows:
> +      // JS_GetTypedArrayByteLength(obj, cx)/JS_GetTypedArrayLength(obj, cx)
> +      *static_cast<void**>(buffer) = JS_GetArrayBufferViewData(valObj, cx);

Please check that the pointer type is consistent (or void *).

Users can easily use ctypes.cast if they really want a reinterpret_cast. Or just pass the ArrayBuffer, which is more like a void*.

@@ +2304,5 @@
> +      // Check that array is consistent with type, then
> +      // copy the array. As with C arrays, data is *not*
> +      // copied back to the ArrayBuffer at the end of a
> +      // function call, so do not expect this to work
> +      // as an inout argument.

ISTR arguments of array type are silently converted to pointers, because that's actually what C does! These two declarations are identical in every way:

    void f(int x[]);
    void f(int *x);

So I think a TypedArray actually *will* work as an in-out argument to such a function, because this particular code won't get called--the pointer code above runs instead.

@@ +2309,5 @@
> +      uint32_t sourceLength = JS_GetTypedArrayByteLength(valObj, cx);
> +      size_t elementSize = CType::GetSize(baseType);
> +      size_t arraySize = elementSize * targetLength;
> +      if (arraySize != size_t(sourceLength)) {
> +        JS_ReportError(cx, "typed array length does not match source array length");

Please check that the types really match, not just that the length is the same. It should be pretty easy; treat a Uint32Array just like a uint32_t.array(length).

::: toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
@@ +1765,5 @@
> +         [new Uint32Array(c_arraybuffer), ctypes.uint32_t],
> +         [new Float32Array(c_arraybuffer), ctypes.float32_t],
> +         [new Float64Array(c_arraybuffer), ctypes.float64_t]
> +       ]) {
> +    let [view, item_type] = sample;

For what it's worth, you can say 'for (let [view, item_type] of ...)'
Comment 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-03 02:39:23 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> ::: toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
> @@ +1765,5 @@
> > +         [new Uint32Array(c_arraybuffer), ctypes.uint32_t],
> > +         [new Float32Array(c_arraybuffer), ctypes.float32_t],
> > +         [new Float64Array(c_arraybuffer), ctypes.float64_t]
> > +       ]) {
> > +    let [view, item_type] = sample;
> 
> For what it's worth, you can say 'for (let [view, item_type] of ...)'

Good point.
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-03 02:54:50 PDT
Created attachment 667403 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v2

Attaching a new version that performs strict type-checking.
Note that we cannot convert a Int32Array to a int[] or int* even on a platform on which int is 32 bits. I believe that allowing this would be a misfeature, what do you think?
Comment 6 Jason Orendorff [:jorendorff] 2012-10-03 07:01:42 PDT
Comment on attachment 667403 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v2

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

It still doesn't check for exact type match when converting a typedarray to a ctypes array. r=me with that.

::: js/src/ctypes/CTypes.cpp
@@ +2202,5 @@
> +        TypeCode elementTypeCode;
> +        switch (JS_GetTypedArrayType(valObj, cx)) {
> +        case TypedArray::TYPE_INT8:
> +          elementTypeCode = TYPE_int8_t;
> +          break;

Factor this code into a function and use the same function in the array case.
Comment 7 Jason Orendorff [:jorendorff] 2012-10-03 07:02:20 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> Attaching a new version that performs strict type-checking.
> Note that we cannot convert a Int32Array to a int[] or int* even on a
> platform on which int is 32 bits. I believe that allowing this would be a
> misfeature, what do you think?

It'd be inconsistent with the existing design. I'm ok with revisiting that design decision later.
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-03 10:09:52 PDT
Created attachment 667531 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v3

Ah, my bad, forgot to check type-conversion with arrays.
Attaching a new version. I took the opportunity to improve testing of ArrayBuffer/typed array to array conversions.
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-03 11:39:36 PDT
Created attachment 667577 [details] [diff] [review]
Feeding typed arrays to js-ctypes, v3

Same patch, *after* qref.
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-05 07:33:27 PDT
Created attachment 668435 [details] [diff] [review]
2. More tests

I just realized that my previous tests fail to cover the critical case in which the typed array does not start at offset 0 of the buffer. Here are the additional tests.
Comment 11 Jason Orendorff [:jorendorff] 2012-10-05 09:34:05 PDT
Comment on attachment 668435 [details] [diff] [review]
2. More tests

Gold star.

Note You need to log in before you can comment on or make changes to this bug.