DataView implementation.

DataView implementation.

R=rossberg@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=15269

[Dmitry Lomov (no reviews), 2013-06-20 13:52:24]

Andreas, please take a look.
This is a DataView implementation, following mostly the Khronos spec.

[rossberg, 2013-06-21 08:44:01]

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#oldco...
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#newco...
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-weaktyped...
File test/cctest/test-weaktypedarrays.cc (right):

https://codereview.chromium.org/17153011/diff/2001/test/cctest/test-weaktyped...
test/cctest/test-weaktypedarrays.cc:378: 
Nit: 2 lines

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
File test/mjsunit/harmony/dataview-accessors.js (right):

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
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/datav...
test/mjsunit/harmony/dataview-accessors.js:40: {
Nit: style violation (here and below)

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
test/mjsunit/harmony/dataview-accessors.js:41: switch (func) {
Nit: indentation style

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
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/datav...
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/datav...
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/datav...
test/mjsunit/harmony/dataview-accessors.js:88: else
Nit: odd indentation

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
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/datav...
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/typed...
File test/mjsunit/harmony/typedarrays.js (right):

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/typed...
test/mjsunit/harmony/typedarrays.js:518: function
TestDataViewPropertyTypeChecks() {
Is this ever invoked?

[Dmitry Lomov (no reviews), 2013-06-21 11:32:10]

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#oldco...
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#newco...
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-weaktyped...
File test/cctest/test-weaktypedarrays.cc (right):

https://codereview.chromium.org/17153011/diff/2001/test/cctest/test-weaktyped...
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/datav...
File test/mjsunit/harmony/dataview-accessors.js (right):

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
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/datav...
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/datav...
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/datav...
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/datav...
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/datav...
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/datav...
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/datav...
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/datav...
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/typed...
File test/mjsunit/harmony/typedarrays.js (right):

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/typed...
test/mjsunit/harmony/typedarrays.js:518: function
TestDataViewPropertyTypeChecks() {
On 2013/06/21 08:44:01, rossberg wrote:
> Is this ever invoked?

Done.

[rossberg, 2013-06-21 12:25:53]

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
File test/mjsunit/harmony/dataview-accessors.js (right):

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
test/mjsunit/harmony/dataview-accessors.js:64: if (index < 0) index = 0;
On 2013/06/21 11:32:10, Dmitry Lomov (chromium) wrote:
> On 2013/06/21 08:44:01, rossberg wrote:
> > I don't understand. What use is runNegativeIndexTest below if you correct it
> > anyway?
> 
> Done.

This doesn't seem to be fixed.

https://codereview.chromium.org/17153011/diff/15001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/17153011/diff/15001/src/runtime.cc#newcode1035
src/runtime.cc:1035: Handle<JSDataView> data_view(JSDataView::cast(*holder));
You can just declare the type in the line above now. (here and below)

https://codereview.chromium.org/17153011/diff/15001/test/mjsunit/harmony/data...
File test/mjsunit/harmony/dataview-accessors.js (right):

https://codereview.chromium.org/17153011/diff/15001/test/mjsunit/harmony/data...
test/mjsunit/harmony/dataview-accessors.js:96: {
Nit: style

[Dmitry Lomov (no reviews), 2013-06-21 12:39:20]

More comments addressed.

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
File test/mjsunit/harmony/dataview-accessors.js (right):

https://codereview.chromium.org/17153011/diff/2001/test/mjsunit/harmony/datav...
test/mjsunit/harmony/dataview-accessors.js:64: if (index < 0) index = 0;
On 2013/06/21 12:25:53, rossberg wrote:
> On 2013/06/21 11:32:10, Dmitry Lomov (chromium) wrote:
> > On 2013/06/21 08:44:01, rossberg wrote:
> > > I don't understand. What use is runNegativeIndexTest below if you correct
it
> > > anyway?
> > 
> > Done.
> 
> This doesn't seem to be fixed.

No it is fixed - see the latest version of this file.

https://codereview.chromium.org/17153011/diff/15001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/17153011/diff/15001/src/runtime.cc#newcode1035
src/runtime.cc:1035: Handle<JSDataView> data_view(JSDataView::cast(*holder));
On 2013/06/21 12:25:53, rossberg wrote:
> You can just declare the type in the line above now. (here and below)

Done.

https://codereview.chromium.org/17153011/diff/15001/test/mjsunit/harmony/data...
File test/mjsunit/harmony/dataview-accessors.js (right):

https://codereview.chromium.org/17153011/diff/15001/test/mjsunit/harmony/data...
test/mjsunit/harmony/dataview-accessors.js:96: {
On 2013/06/21 12:25:53, rossberg wrote:
> Nit: style

Done.

[rossberg, 2013-06-21 12:50:07]

lgtm

[Dmitry Lomov (no reviews), 2013-06-21 13:03:02]

Committed patchset #5 manually as r15269 (presubmit successful).