LGTM with comments.
http://codereview.chromium.org/9114050/diff/9004/src/d8.cc File src/d8.cc (right): http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode399 src/d8.cc:399: if (offset + (length * element_size) > array_buffer_length) { You need an equivalent check (offset > array_buffer_length) in the args.Length() == 2 case. http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode407 src/d8.cc:407: String::New("ArrayBuffer minus the byteOffset must be a " nit: s/ArrayBuffer/ArrayBuffer length/ http://codereview.chromium.org/9114050/diff/9004/src/d8.cc#newcode456 src/d8.cc:456: return result; nit: you don't need this line, could return directly instead (as the next couple of functions do). http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array.js File test/mjsunit/external-array.js (right): http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array.js#newcode1 test/mjsunit/external-array.js:1: // Copyright 2011 the V8 project authors. All rights reserved. 2012 http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array.js#newcode75 test/mjsunit/external-array.js:75: new Uint32Array(ab,13); s/13/16/ to avoid triggering the "offset must be multiple of element size" code path. http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array.js#newcode81 test/mjsunit/external-array.js:81: function abfunc5() { nit: AFAICS, abfunc5 and abfunc2 test the same thing. http://codereview.chromium.org/9114050/diff/9004/test/mjsunit/external-array.js#newcode91 test/mjsunit/external-array.js:91: new Uint32Array(ab,1); This doesn't test what the comment says (13-1 is, in fact, a multiple of 4, but an exception is thrown nevertheless, because the offset must be a multiple of the element size too). What you mean is "new Uint32Array(ab, 4);". http://codereview.chromium.org/9114050/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
