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());
Why not CHECK(IsJSArrayBufferView())?
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:
Nit: 2 empty lines
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) \
Please add the new classes to the hierarchy comment near the top of the
file
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()) {
Why not move this check to the JavaScript side? (here and below)
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1080
src/runtime.cc:1080: uint8_t temp = *p;
Nit: indentation
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1139
src/runtime.cc:1139: T data;
Nit: indentation
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1148
src/runtime.cc:1148: OS::MemCopy(value.bytes,
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. :)
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1177
src/runtime.cc:1177: T data;
Nit: indentation
https://codereview.chromium.org/17153011/diff/2001/src/runtime.cc#newcode1196
src/runtime.cc:1196: #define DATA_VIEW_GETTER(TypeName, Type, Convertor)
\
Nit: I think the spelling is Converter
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',[]);
Nit: space after comma
https://codereview.chromium.org/17153011/diff/2001/src/typedarray.js#newcode244
src/typedarray.js:244: var little_endian = IS_UNDEFINED(little_endian)
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)
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:
Nit: 2 lines
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];
Nit: line length
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode40
test/mjsunit/harmony/dataview-accessors.js:40: {
Nit: style violation (here and below)
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode41
test/mjsunit/harmony/dataview-accessors.js:41: switch (func) {
Nit: indentation style
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");
I think we have assertUnreachable()
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;
I don't understand. What use is runNegativeIndexTest below if you
correct it anyway?
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode80
test/mjsunit/harmony/dataview-accessors.js:80: } else
Nit: style guide wants braces consistently on branches
https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/dataview-accessors.js#newcode88
test/mjsunit/harmony/dataview-accessors.js:88: else
Nit: odd indentation
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;
Shouldn't the DataView constructor do all the defaulting for you?
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;
Nit: line length
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() {
Is this ever invoked?
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.
