Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Added slice() support to ArrayBuffer + tests #4130

Closed
wants to merge 1 commit into from

3 participants

@inolen

This patch adds in slice() support to ArrayBuffer as per:
http://www.khronos.org/registry/typedarray/specs/latest/#5

I'm working on some code that's shared on the client and server and this became an issue.

I wasn't entirely sure of the purpose of the signature parameter to v8::FunctionTemplate::New, so I followed suit with what TypedArray and DataView were doing right below in the same file. Everything else seemed straight forward but this is my first time working with v8 / node's source.

Tests are attached as well.

@langpavel langpavel commented on the diff
src/v8_typed_array.cc
@@ -71,6 +71,13 @@ class ArrayBuffer {
v8::Local<v8::ObjectTemplate> instance = ft_cache->InstanceTemplate();
instance->SetInternalFieldCount(1); // Buffer.
+ v8::Local<v8::Signature> default_signature = v8::Signature::New(ft_cache);
+
+ instance->Set(v8::String::New("slice"),

String::NewSymbol("slice")

That's only useful if you're creating the same string many times, which isn't the case here. Frankly, we're overusing (abusing) String::NewSymbol() a great deal in node.

Oh, thank for explain.. but still, there is not "slice" symbol in core? I think yes (but don't verify) :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on the diff
src/v8_typed_array.cc
@@ -139,6 +146,42 @@ class ArrayBuffer {
return args.This();
}
+
+ static v8::Handle<v8::Value> slice(const v8::Arguments& args) {
+ if (args.Length() < 1)
+ return ThrowError("Wrong number of arguments.");
+
+ unsigned int length =
+ args.This()->Get(v8::String::New("byteLength"))->Uint32Value();

Use args.This()->GetIndexedPropertiesExternalArrayDataLength() here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis bnoordhuis commented on the diff
src/v8_typed_array.cc
((21 lines not shown))
+ if (end < 0) end = 0;
+ if ((unsigned)end > length) end = length;
+
+ if (begin > end) begin = end;
+
+ unsigned int slice_length = end - begin;
+ v8::Local<v8::Value> argv[] = {
+ v8::Integer::New(slice_length)};
+ v8::Local<v8::Object> buffer = ArrayBuffer::GetTemplate()->
+ GetFunction()->NewInstance(1, argv);
+
+ if (buffer.IsEmpty()) return v8::Undefined(); // constructor failed
+
+ void* src = args.This()->GetPointerFromInternalField(0);
+ void* dest = buffer->GetPointerFromInternalField(0);
+ memcpy(dest, reinterpret_cast<char*>(src) + begin, slice_length);

static_cast?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bnoordhuis

Minor nits but otherwise LGTM. Can you sign the CLA?

By the way, are you the same inolen that (used to?) frequent the Q3W forums?

@inolen

Signed and yup. Crossed your name with an ioq3 patch - small world :)

Thanks for the review. Should I push the changes you mentioned in the notes or is that something that'd be done by whoever merges this?

@bnoordhuis

Thanks Anthony, landed with some minor static_cast touch-ups in fddb5dc.

Should I push the changes you mentioned in the notes or is that something that'd be done by whoever merges this?

Push the changes to your feature branch and post a "all fixed" message. The reviewer (that's me, generally speaking) will squash them.

@bnoordhuis bnoordhuis closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 13, 2012
  1. Added slice() support to ArrayBuffers + tests

    Anthony Pesch authored
This page is out of date. Refresh to see the latest.
Showing with 132 additions and 0 deletions.
  1. +43 −0 src/v8_typed_array.cc
  2. +89 −0 test/simple/test-arraybuffer-slice.js
View
43 src/v8_typed_array.cc
@@ -71,6 +71,13 @@ class ArrayBuffer {
v8::Local<v8::ObjectTemplate> instance = ft_cache->InstanceTemplate();
instance->SetInternalFieldCount(1); // Buffer.
+ v8::Local<v8::Signature> default_signature = v8::Signature::New(ft_cache);
+
+ instance->Set(v8::String::New("slice"),

String::NewSymbol("slice")

That's only useful if you're creating the same string many times, which isn't the case here. Frankly, we're overusing (abusing) String::NewSymbol() a great deal in node.

Oh, thank for explain.. but still, there is not "slice" symbol in core? I think yes (but don't verify) :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ v8::FunctionTemplate::New(&ArrayBuffer::slice,
+ v8::Handle<v8::Value>(),
+ default_signature));
+
return ft_cache;
}
@@ -139,6 +146,42 @@ class ArrayBuffer {
return args.This();
}
+
+ static v8::Handle<v8::Value> slice(const v8::Arguments& args) {
+ if (args.Length() < 1)
+ return ThrowError("Wrong number of arguments.");
+
+ unsigned int length =
+ args.This()->Get(v8::String::New("byteLength"))->Uint32Value();

Use args.This()->GetIndexedPropertiesExternalArrayDataLength() here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ int begin = args[0]->Int32Value();
+ int end = length;
+ if (args.Length() > 1)
+ end = args[1]->Int32Value();
+
+ if (begin < 0) begin = length + begin;
+ if (begin < 0) begin = 0;
+ if ((unsigned)begin > length) begin = length;
+
+ if (end < 0) end = length + end;
+ if (end < 0) end = 0;
+ if ((unsigned)end > length) end = length;
+
+ if (begin > end) begin = end;
+
+ unsigned int slice_length = end - begin;
+ v8::Local<v8::Value> argv[] = {
+ v8::Integer::New(slice_length)};
+ v8::Local<v8::Object> buffer = ArrayBuffer::GetTemplate()->
+ GetFunction()->NewInstance(1, argv);
+
+ if (buffer.IsEmpty()) return v8::Undefined(); // constructor failed
+
+ void* src = args.This()->GetPointerFromInternalField(0);
+ void* dest = buffer->GetPointerFromInternalField(0);
+ memcpy(dest, reinterpret_cast<char*>(src) + begin, slice_length);

static_cast?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ return buffer;
+ }
};
static bool checkAlignment(size_t val, unsigned int bytes) {
View
89 test/simple/test-arraybuffer-slice.js
@@ -0,0 +1,89 @@
+// Copyright Joyent, Inc. and other Node contributors.
+
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+/*
+ * Tests to verify slice functionality of ArrayBuffer.
+ * (http://www.khronos.org/registry/typedarray/specs/latest/#5)
+ */
+
+var common = require('../common');
+var assert = require('assert');
+
+var buffer = new ArrayBuffer(8);
+var sub;
+var i;
+
+for (var i = 0; i < 8; i++) {
+ buffer[i] = i;
+}
+
+// slice a copy of buffer
+sub = buffer.slice(2, 6);
+
+// make sure it copied correctly
+assert.ok(sub instanceof ArrayBuffer);
+assert.equal(sub.byteLength, 4);
+
+for (i = 0; i < 4; i++) {
+ assert.equal(sub[i], i+2);
+}
+
+// modifications to the new slice shouldn't affect the original
+sub[0] = 999;
+
+for (i = 0; i < 8; i++) {
+ assert.equal(buffer[i], i);
+}
+
+// test optional end param
+sub = buffer.slice(5);
+
+assert.ok(sub instanceof ArrayBuffer);
+assert.equal(sub.byteLength, 3);
+for (i = 0; i < 3; i++) {
+ assert.equal(sub[i], i+5);
+}
+
+// test clamping
+sub = buffer.slice(-3, -1);
+
+assert.ok(sub instanceof ArrayBuffer);
+assert.equal(sub.byteLength, 2);
+for (i = 0; i < 2; i++) {
+ assert.equal(sub[i], i+5);
+}
+
+// test invalid clamping range
+sub = buffer.slice(-1, -3);
+
+assert.ok(sub instanceof ArrayBuffer);
+assert.equal(sub.byteLength, 0);
+
+// test wrong number of arguments
+var gotError = false;
+
+try {
+ sub = buffer.slice();
+} catch (e) {
+ gotError = true;
+}
+
+assert.ok(gotError);
Something went wrong with that request. Please try again.