Ben,
I appreciate the work you put into this patch.
However, I think this implementation is a little premature: 1. DataView is
not
yet very well specified in ES6 draft
2. much of the implementation of typed arrays/array buffers etc.,
especially on
API side, might change significantly while we are integrating this in
Blink. On
implementation side, one thing that is not finished at all yet is
ArrayBuffer
neutering, and that will affect many implementation details here.
If you noticed, I have been implementing ArrayBuffers and friends in V8,
and I
postponed tackling DataView for precisely these reasons.
If you feel like continuing working on this patch, here are a few bigger
things
that would be needed:
1. Please split this patch into two parts: one concerning JavaScript side of
things (implementation of constructor and methods), and another concerning
V8
API (include/v8.h, api.h etc).
2. Generally there should be a base class for JSTypedArray an JSDataView,
such
as JSArrayBufferView, that shares byte_offset, byte_length and buffer.
2. Please add way more tests. I put some suggestions below, also you might
want
to take a look at WebKit layout tests.
Also, my understanding is that your tests will not work on big-endian
machine.
I put together some of the more important comments for your code below; I
mostly
looked at JS part, not at the API part.
https://codereview.chromium.org/15943002/diff/17001/src/api.cc
File src/api.cc (right):
https://codereview.chromium.org/15943002/diff/17001/src/api.cc#newcode6099
src/api.cc:6099: i::Handle<i::ExternalArray> elements =
DataView is not a external array, so everything here is not needed.
https://codereview.chromium.org/15943002/diff/17001/src/dataview.js
File src/dataview.js (right):
https://codereview.chromium.org/15943002/diff/17001/src/dataview.js#newcode82
src/dataview.js:82: var offset = TO_INT32(byteOffset);
Should be TO_POSITIVE_INTEGER. Byte offsets may be longer than int32.
Also in all getters below.
https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h#newcode5335
src/objects-inl.h:5335: size_t byte_length =
static_cast<size_t>(this->byte_length()->Number());
Use NumberToSize
https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h#newcode5353
src/objects-inl.h:5353: size_t byte_length =
static_cast<size_t>(this->byte_length()->Number());
Use NumberToSize.
https://codereview.chromium.org/15943002/diff/17001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/15943002/diff/17001/src/runtime.cc#newcode795
src/runtime.cc:795: holder->set_map(*map);
This is completely unneeded. DataViews are not external arrays. They do
not have a redefined notion of indexing operation:
var dv = new DataView(new ArrayBuffer(100));
dv[0] = "a";
dv[0] // "a"
https://codereview.chromium.org/15943002/diff/17001/src/runtime.cc#newcode828
src/runtime.cc:828: CONVERT_SMI_ARG_CHECKED(byte_offset, 1);
This is wrong. Even if byte_offset is a result of TO_INT32, it won't fit
into SMI. The proper implementation should handle byte offest bigger
than int 32. Same applies throughout.
https://codereview.chromium.org/15943002/diff/17001/src/utils.h
File src/utils.h (right):
https://codereview.chromium.org/15943002/diff/17001/src/utils.h#newcode267
src/utils.h:267: inline enum Endianness GetEndianness() {
This belongs to cpu.h. Should be a #define with possibly STATIC_ASSERT,
not a runtime function
https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/dataview.js
File test/mjsunit/harmony/dataview.js (right):
https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/dataview.js#newcode1
test/mjsunit/harmony/dataview.js:1: // Copyright 2013 the V8 project
authors. All rights reserved.
This needs way more tests, including calling constructor with "new", out
of range arguments for the constructor, behavior of DataView as a
regular JS object (adding properties, indexed access etc)
https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/dataview.js#newcode30
test/mjsunit/harmony/dataview.js:30: assertThrows("DataView()",
TypeError);
Please do not use string version of assertThrows, use the version that
takes a a function:
assertThrows(function() { DataView(); }, TypeError);
https://codereview.chromium.org/15943002/
--
--
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.
