Thanks a lot for thorough - and fast! - review. Comments addressed, PTAL.
https://codereview.chromium.org/17153011/diff/2001/src/objects-debug.cc
File src/objects-debug.cc (left):
https://codereview.chromium.org/17153011/diff/2001/src/objects-debug.cc#oldcode765
src/objects-debug.cc:765: CHECK(IsJSTypedArray());
On 2013/06/21 08:44:01, rossberg wrote:
Why not CHECK(IsJSArrayBufferView())?
Done.
https://codereview.chromium.org/17153011/diff/2001/src/objects-debug.cc
File src/objects-debug.cc (right):
https://codereview.chromium.org/17153011/diff/2001/src/objects-debug.cc#newcode780
src/objects-debug.cc:780:
On 2013/06/21 08:44:01, rossberg wrote:
Nit: 2 empty lines
Done.
https://codereview.chromium.org/17153011/diff/2001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/17153011/diff/2001/src/objects.h#newcode998
src/objects.h:998: V(JSArrayBufferView) \
On 2013/06/21 08:44:01, rossberg wrote:
Please add the new classes to the hierarchy comment near the top of
the file
Done.
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1035
src/runtime.cc:1035: if (!holder->IsJSDataView()) {
On 2013/06/21 08:44:01, rossberg wrote:
Why not move this check to the JavaScript side? (here and below)
Done.
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1080
src/runtime.cc:1080: uint8_t temp = *p;
On 2013/06/21 08:44:01, rossberg wrote:
Nit: indentation
Done.
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1139
src/runtime.cc:1139: T data;
On 2013/06/21 08:44:01, rossberg wrote:
Nit: indentation
Done. Didn't move this to v8utils.h, because it looks like a single-use
utility.
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1148
src/runtime.cc:1148: OS::MemCopy(value.bytes,
On 2013/06/21 08:44:01, rossberg wrote:
OS::MemCopy seems overkill here. Use at least the CopyBytes function
from
v8utils, which special-cases small sizes. Or even better, add some
abstraction
to v8utils that handles single values super efficiently. :)
Done.
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1177
src/runtime.cc:1177: T data;
On 2013/06/21 08:44:01, rossberg wrote:
Nit: indentation
Done.
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1196
src/runtime.cc:1196: #define DATA_VIEW_GETTER(TypeName, Type, Convertor)
\
On 2013/06/21 08:44:01, rossberg wrote:
Nit: I think the spelling is Converter
Done.
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1196
src/runtime.cc:1196: #define DATA_VIEW_GETTER(TypeName, Type, Convertor)
\
On 2013/06/21 08:44:01, rossberg wrote:
Nit: I think the spelling is Converter
Done.
https://codereview.chromium.org/17153011/diff/2001/src/typedarray.js
File src/typedarray.js (right):
https://codereview.chromium.org/17153011/diff/2001/src/typedarray.js#newcode214
src/typedarray.js:214: throw
MakeRangeError('invalid_data_view_offset',[]);
On 2013/06/21 08:44:01, rossberg wrote:
Nit: space after comma
Done.
https://codereview.chromium.org/17153011/diff/2001/src/typedarray.js#newcode244
src/typedarray.js:244: var little_endian = IS_UNDEFINED(little_endian)
On 2013/06/21 08:44:01, rossberg wrote:
Don't use redundant 'var' (here and below).
Also, why do you need to special-case 'undefined'? Moreover, why not
simply use
the common JavaScript idiom: x = !!x
Can't all this just be a single line like
return %DataViewGetInt8(this, TO_POSITIVE_INTEGER(offset),
!!little_endian)
Done.
https://codereview.chromium.org/17153011/diff/2001/test/cctest/test-weaktypedarrays.cc
File test/cctest/test-weaktypedarrays.cc (right):
https://codereview.chromium.org/17153011/diff/2001/test/cctest/test-weaktypedarrays.cc#newcode378
test/cctest/test-weaktypedarrays.cc:378:
On 2013/06/21 08:44:01, rossberg wrote:
Nit: 2 lines
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js
File test/mjsunit/harmony/dataview-accessors.js (right):
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode30
test/mjsunit/harmony/dataview-accessors.js:30: var intArray1 = [0, 1, 2,
3, 100, 101, 102, 103, 128, 129, 130, 131, 252, 253, 254, 255];
On 2013/06/21 08:44:01, rossberg wrote:
Nit: line length
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode40
test/mjsunit/harmony/dataview-accessors.js:40: {
On 2013/06/21 08:44:01, rossberg wrote:
Nit: style violation (here and below)
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode41
test/mjsunit/harmony/dataview-accessors.js:41: switch (func) {
On 2013/06/21 08:44:01, rossberg wrote:
Nit: indentation style
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode55
test/mjsunit/harmony/dataview-accessors.js:55: debug("Should not
reached");
On 2013/06/21 08:44:01, rossberg wrote:
I think we have assertUnreachable()
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode64
test/mjsunit/harmony/dataview-accessors.js:64: if (index < 0) index = 0;
On 2013/06/21 08:44:01, rossberg wrote:
I don't understand. What use is runNegativeIndexTest below if you
correct it
anyway?
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode80
test/mjsunit/harmony/dataview-accessors.js:80: } else
On 2013/06/21 08:44:01, rossberg wrote:
Nit: style guide wants braces consistently on branches
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode88
test/mjsunit/harmony/dataview-accessors.js:88: else
On 2013/06/21 08:44:01, rossberg wrote:
Nit: odd indentation
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode98
test/mjsunit/harmony/dataview-accessors.js:98: viewStart = (start !=
undefined) ? start : 0;
On 2013/06/21 08:44:01, rossberg wrote:
Shouldn't the DataView constructor do all the defaulting for you?
Done.
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode99
test/mjsunit/harmony/dataview-accessors.js:99: viewLength = (length !=
undefined) ? length : arrayBuffer.byteLength - viewStart;
On 2013/06/21 08:44:01, rossberg wrote:
Nit: line length
Done by removing line
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/typedarrays.js
File test/mjsunit/harmony/typedarrays.js (right):
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/typedarrays.js#newcode518
test/mjsunit/harmony/typedarrays.js:518: function
TestDataViewPropertyTypeChecks() {
On 2013/06/21 08:44:01, rossberg wrote:
Is this ever invoked?
Done.
https://codereview.chromium.org/17153011/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
