Please take another look
http://codereview.chromium.org/9114050/diff/4001/src/d8.cc File src/d8.cc (right): http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode284 src/d8.cc:284: static int convertToInt(Local<Value> value_in, TryCatch* try_catch) { On 2012/01/11 10:17:53, Jakob wrote:
Rename to convertToUint?
Done. http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode339 src/d8.cc:339: if (args.Length() > 3 || args.Length() == 0) { On 2012/01/11 10:17:53, Jakob wrote:
Arguments.operator[] is benign enough to simply return 'undefined' for
args[0]
if args.Length == 0, but nevertheless I'd prefer to check for Length() 0 before doing anything else (you can print a better error message that
way, too). Done. http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode350 src/d8.cc:350: : args[2]; On 2012/01/11 10:17:53, Jakob wrote:
args[2] is optional and could be undefined. As implemented, the code
defaults to
0 in that case, which is not correct.
Done. http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode363 src/d8.cc:363: array->Get(String::New("BYTES_PER_ELEMENT"))->ToInt32()->Int32Value(); On 2012/01/11 10:17:53, Jakob wrote:
My reading of the spec at http://www.khronos.org/registry/typedarray/specs/latest/#7 is that
|array| must
be an ArrayBuffer (and cannot be another kind of TypedArray), so BYTES_PER_ELEMENTS is always 1, and you'll have to throw an exception
if a
TypedArray is passed in. Further, IIUC, the "length" parameter specifies the new array's length
in terms
of number of elements in the new array, so all this conversion-fu is unnecessary.
Done. http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode389 src/d8.cc:389: // Hold a reference to the ArrayBuffer so it's buffer doesn't get collected. On 2012/01/11 10:17:53, Jakob wrote:
s/it's/its/
Done. http://codereview.chromium.org/9114050/diff/4001/src/d8.cc#newcode393 src/d8.cc:393: reinterpret_cast<uint8_t*>(data) + offset, type, On 2012/01/11 10:17:53, Jakob wrote:
Whoa... please add checks that both |offset| and |offset +
length*element_size|
are within the existing |data| buffer.
Done. http://codereview.chromium.org/9114050/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
