First step towards implementing dart:typeddata library.

First step towards implementing dart:typeddata library.
Committed: https://code.google.com/p/dart/source/detail?r=19427

[Lasse Reichstein Nielsen, 2013-02-25 12:02:50]

drive-by comments on the library part.

Is this a remake of dart:scalarlist?

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:15: int lengthInBytes();
Should be a getter.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:29: int elementSizeInBytes();
Also getter, and same for the ones below.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:65: *     int huh = ba.getInt64(0);
Should it be mentioned here that the code doesn't work in dart2js (since there
are no 64-bit integers)?

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:94: * if `endIndex - startIndex` is
greater than the length of this object.
or if startIndex or endIndex are greater than the length, or if endIndex is less
than startIndex.

Maybe we should have a definition of startIndex and endIndex "representing a
valid subrange of this" that we can refer to instead of writing it all out every
time.

[Anton Muhin, 2013-02-25 20:11:20]

DBQ

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:569: external factory
Uint32List.view(ByteBuffer buffer,
there are no .transferable constructors, is it by design?

[sra1, 2013-02-26 07:25:30]

DBC

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:9: * numerical data more efficiently using
a typed view.
I don't really think of bitmaps and textures a 'numerical'.
Perhaps 'numerical and binary'.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:10: */
I would say that this is the sequence of bytes underlying a typed data object.

(In some ways 'Allocation' might be a better name for this class.)

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:15: int lengthInBytes();
'byteLength' would be more familiar to users of DOM typed arrays.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:23: abstract class TypedData {
'TypedData' is a bit of a strange name. In Dart, all values, hence all data, is
typed.  This library allows you to view bytes as several types, so in a sense,
this in *untyped data*.
I don't have a good suggestion, perhaps 'BinaryData'.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:27: * object if it is not a list.
So elementSizeInBytes for a ByteData is not 1?

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:29: int elementSizeInBytes();
'bytesPerElement' is shorter and would be more familiar to users of DOM typed
arrays.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:64: *     ba.setFloat64(0,
3.14159265358979323846);
ba -> bdata

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:65: *     int huh = ba.getInt64(0);
On 2013/02/25 12:02:51, Lasse Reichstein Nielsen wrote:
> Should it be mentioned here that the code doesn't work in dart2js (since there
> are no 64-bit integers)

I would suggest

(1) change the example to a 32-bit example so the inevitable documentation of
what works is restricted to the classes and members that have special issues.

(2) make the example more meaningful.  E.g. extract the exponent and de-bias to
compute floorLog2.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:150: * greater than 255.
This check is not done by DataView, so it will be expensive to implement for
dart2js.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:157: * form. The return value will be
between 2<sup>15</sup> and 2<sup>15 - 1,
Missing </sup>, also more below.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:161: * `byteOffset + 2` is greater than
the length of this object.
Endianness needs to be specified.

I assume that this operation is host-endian (compatible with Int16List), which
is unsuitable for parsing data sent by a server.  A network format will specify
endianness.

DOM can efficiently support explicit big-endian and little-endian.

host-endian is more difficult. I think at best we will have to dynamically
select between a big-endian and little-endian implementations of ByteData.  That
will inhibit static inlining and add unused methods to every download, but a
good JavaScript implementation should have something to work with for inlining.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:480: * [start] of the region is not
divisible by 2.
It took me a long time to see that [start] was aligned.

1. Call out each constraint in a separate sentence or paragraph so it is not so
easy to miss.

2. Except where there is a good reason, put constraints in the same order as the
arguments they constrain.

Missing from the description is the constraint that the view fits within the
buffer.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:481: */
This 'length' is different to the other constructor's 'length'.

We can make the naming crisper by either (1) changing the API to take a length
counted in elements or (2) rename the parameter to make it obvious that the
argument is a count of bytes.

Counting in elements has the advantage that there can't be an ArgumentError due
to 'alignment' of the length.

The DOM names for the parameters to the analogous Int16Array constructor are
'byteOffset' and 'length'.

Unless there is a strong reason to do otherwise, I suggest following the DOM
here to avoid arbitrary incompatibility.  Even small differences like having to
convert a byte length to an element length and do the alignment check requires
additional code, and I don't think there is clear winner in terms of which is
the more readable code.

Is there a good reason not to go with the DOM names?
byteOffset
byteLength
length
bytesPerElement (BYTES_PER_ELEMENT)

The new names are longer but don't really add much: 
offsetInBytes
lengthInBytes
length
elementSizeInBytes

One advantage of keeping the same names for members is that the name in
exception messages are closer to the source.  If we inline Dart
"x.offsetInBytes" to  JS "x.byteOffset", and x is null, the message will contain
'byteOffset', not 'offsetInBytes'.  It would be less confusing to debug the JS
if they were both 'byteOffset'.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:483: [int start = 0, int length]);
Other things being equal, a constructor for an object with properties 'a' and
'b' where the values of 'a' and 'b' are determined by parameters to the
constructor should have parameters named 'a' and 'b'.

If we stick with the current getter names, rename this to 

[int offsetInBytes = 0, int lengthInBytes]

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:484: }
The JS class/constructor, Int16Array, has a static constant BYTES_PER_ELEMENT,
which can be used to divide a byteLength to get an element length.

Are we going to provide something similar to enable expressions like the
following?

length = byteLength ~/ Int16List.BYTES_PER_ELEMENT;
byteLength = length * Int16List.BYTES_PER_ELEMENT;

[Ivan Posva, 2013-02-26 23:26:20]

First set of comments.

-Ivan

https://codereview.chromium.org/12313088/diff/7019/runtime/lib/typeddata.dart
File runtime/lib/typeddata.dart (right):

https://codereview.chromium.org/12313088/diff/7019/runtime/lib/typeddata.dart...
runtime/lib/typeddata.dart:255: abstract class _TypedDataBase {
Shouldn't this be _TypedListBase? As not all implementations of TypedData are
going to inherit from here.

https://codereview.chromium.org/12313088/diff/7019/runtime/lib/typeddata.dart...
runtime/lib/typeddata.dart:456: abstract class _TypedData extends _TypedDataBase
ditto

https://codereview.chromium.org/12313088/diff/7019/sdk/lib/typeddata/typeddat...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://codereview.chromium.org/12313088/diff/7019/sdk/lib/typeddata/typeddat...
sdk/lib/typeddata/typeddata_base.dart:150: * greater than 255.
We had a agreed on silently truncating for all operations, except of course
where truncating is replaced with clamping.

https://codereview.chromium.org/12313088/diff/7019/sdk/lib/typeddata/typeddat...
sdk/lib/typeddata/typeddata_base.dart:161: * `byteOffset + 2` is greater than
the length of this object.
In this first cut we left out endian-specific API to cut down on the surface
area.

On 2013/02/26 07:25:30, sra1 wrote:
> Endianness needs to be specified.
> 
> I assume that this operation is host-endian (compatible with Int16List), which
> is unsuitable for parsing data sent by a server.  A network format will
specify
> endianness.
> 
> DOM can efficiently support explicit big-endian and little-endian.
> 
> host-endian is more difficult. I think at best we will have to dynamically
> select between a big-endian and little-endian implementations of ByteData. 
That
> will inhibit static inlining and add unused methods to every download, but a
> good JavaScript implementation should have something to work with for
inlining.

https://codereview.chromium.org/12313088/diff/7019/sdk/lib/typeddata/typeddat...
sdk/lib/typeddata/typeddata_base.dart:478: * Throws [ArgumentError] if the
length of the specified region
As discussed length is length in elements like in the other constructor. This
documentation from the original scalarlist is outdated.

[siva, 2013-02-27 19:51:51]

synched up to TOT and resolved code to the new library initialization mechanism
in the VM.

Addressed comments.

PTAL.

https://chromiumcodereview.appspot.com/12313088/diff/7019/runtime/lib/typedda...
File runtime/lib/typeddata.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/7019/runtime/lib/typedda...
runtime/lib/typeddata.dart:255: abstract class _TypedDataBase {
Renamed it _TypedListBase

On 2013/02/26 23:26:20, Ivan Posva wrote:
> Shouldn't this be _TypedListBase? As not all implementations of TypedData are
> going to inherit from here.

https://chromiumcodereview.appspot.com/12313088/diff/7019/runtime/lib/typedda...
runtime/lib/typeddata.dart:456: abstract class _TypedData extends _TypedDataBase
Renamed _TypedData to _TypedList and _TypedDataBase to _TypedListBase
On 2013/02/26 23:26:20, Ivan Posva wrote:
> ditto

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:9: * numerical data more efficiently using
a typed view.
Reworded the comment.

On 2013/02/26 07:25:30, sra1 wrote:
> I don't really think of bitmaps and textures a 'numerical'.
> Perhaps 'numerical and binary'.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:10: */
Reworded the comment.

On 2013/02/26 07:25:30, sra1 wrote:
> I would say that this is the sequence of bytes underlying a typed data object.
> 
> (In some ways 'Allocation' might be a better name for this class.)

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:15: int lengthInBytes();
I seem to prefer more verbose names as it clearly documents the field.

On 2013/02/26 07:25:30, sra1 wrote:
> 'byteLength' would be more familiar to users of DOM typed arrays.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:15: int lengthInBytes();
On 2013/02/25 12:02:51, Lasse Reichstein Nielsen wrote:
> Should be a getter.

Done.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:23: abstract class TypedData {
I think in the g+ discussion there was a lot of back and forth on the name to
use for this class. I think we finally settled on TypedData. At this point I
don't think I want to get into another naming discussion, let us stick with
TypedData and after things are working reasonably well we can re visit.

On 2013/02/26 07:25:30, sra1 wrote:
> 'TypedData' is a bit of a strange name. In Dart, all values, hence all data,
is
> typed.  This library allows you to view bytes as several types, so in a sense,
> this in *untyped data*.
> I don't have a good suggestion, perhaps 'BinaryData'.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:27: * object if it is not a list.
Removed that line in the comment regarding the entire object if it is not a
list, seems incorrect (some left over comment from scalar list).

On 2013/02/26 07:25:30, sra1 wrote:
> So elementSizeInBytes for a ByteData is not 1?

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:29: int elementSizeInBytes();
Ditto comment regarding more verbose names. If there is a general consensus on
naming these the same as DOM typed arrays, I am willing to change these names.

On 2013/02/26 07:25:30, sra1 wrote:
> 'bytesPerElement' is shorter and would be more familiar to users of DOM typed
> arrays.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:29: int elementSizeInBytes();
On 2013/02/25 12:02:51, Lasse Reichstein Nielsen wrote:
> Also getter, and same for the ones below.

Done.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:64: *     ba.setFloat64(0,
3.14159265358979323846);
On 2013/02/26 07:25:30, sra1 wrote:
> ba -> bdata

Done.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:65: *     int huh = ba.getInt64(0);
Changed the example to a 32 bit example to avoid the documentation issues of
dart2js.

On 2013/02/26 07:25:30, sra1 wrote:
> On 2013/02/25 12:02:51, Lasse Reichstein Nielsen wrote:
> > Should it be mentioned here that the code doesn't work in dart2js (since
there
> > are no 64-bit integers)
> 
> I would suggest
> 
> (1) change the example to a 32-bit example so the inevitable documentation of
> what works is restricted to the classes and members that have special issues.
> 
> (2) make the example more meaningful.  E.g. extract the exponent and de-bias
to
> compute floorLog2.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:65: *     int huh = ba.getInt64(0);
I have changed the example to use 32 bit values.
We probably need to document these differences between dart2js and dartVM in a
more centralized place.

On 2013/02/25 12:02:51, Lasse Reichstein Nielsen wrote:
> Should it be mentioned here that the code doesn't work in dart2js (since there
> are no 64-bit integers)?

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:94: * if `endIndex - startIndex` is
greater than the length of this object.
Added conditions
endIndex is less than startIndex, or
startIndex is greater than the length of the object.

(the endIndex greater than the length of the object becomes implicit then).


On 2013/02/25 12:02:51, Lasse Reichstein Nielsen wrote:
> or if startIndex or endIndex are greater than the length, or if endIndex is
less
> than startIndex.
> 
> Maybe we should have a definition of startIndex and endIndex "representing a
> valid subrange of this" that we can refer to instead of writing it all out
every
> time.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:150: * greater than 255.
Removed comment.

On 2013/02/26 07:25:30, sra1 wrote:
> This check is not done by DataView, so it will be expensive to implement for
> dart2js.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:150: * greater than 255.
On 2013/02/26 23:26:20, Ivan Posva wrote:
> We had a agreed on silently truncating for all operations, except of course
> where truncating is replaced with clamping.

Done.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:157: * form. The return value will be
between 2<sup>15</sup> and 2<sup>15 - 1,
On 2013/02/26 07:25:30, sra1 wrote:
> Missing </sup>, also more below.

Done.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:161: * `byteOffset + 2` is greater than
the length of this object.
I have left out the endianness stuff in the first cut. Will add those in the
next CL.

On 2013/02/26 07:25:30, sra1 wrote:
> Endianness needs to be specified.
> 
> I assume that this operation is host-endian (compatible with Int16List), which
> is unsuitable for parsing data sent by a server.  A network format will
specify
> endianness.
> 
> DOM can efficiently support explicit big-endian and little-endian.
> 
> host-endian is more difficult. I think at best we will have to dynamically
> select between a big-endian and little-endian implementations of ByteData. 
That
> will inhibit static inlining and add unused methods to every download, but a
> good JavaScript implementation should have something to work with for
inlining.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:478: * Throws [ArgumentError] if the
length of the specified region
Removed comment and changed the name 'start' to 'offsetInBytes' to make it more
explicit.

On 2013/02/26 23:26:20, Ivan Posva wrote:
> As discussed length is length in elements like in the other constructor. This
> documentation from the original scalarlist is outdated.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:480: * [start] of the region is not
divisible by 2.
[start] is not aligned renamed [start] to [offsetInBytes]

Added a RangeError comment to document the constraint of the view fitting in the
underlying buffer.

On 2013/02/26 07:25:30, sra1 wrote:
> It took me a long time to see that [start] was aligned.
> 
> 1. Call out each constraint in a separate sentence or paragraph so it is not
so
> easy to miss.
> 
> 2. Except where there is a good reason, put constraints in the same order as
the
> arguments they constrain.
> 
> Missing from the description is the constraint that the view fits within the
> buffer.

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:481: */
'length' is length counted in elements.

On 2013/02/26 07:25:30, sra1 wrote:
> This 'length' is different to the other constructor's 'length'.
> 
> We can make the naming crisper by either (1) changing the API to take a length
> counted in elements or (2) rename the parameter to make it obvious that the
> argument is a count of bytes.
> 
> Counting in elements has the advantage that there can't be an ArgumentError
due
> to 'alignment' of the length.
> 
> The DOM names for the parameters to the analogous Int16Array constructor are
> 'byteOffset' and 'length'.
> 
> Unless there is a strong reason to do otherwise, I suggest following the DOM
> here to avoid arbitrary incompatibility.  Even small differences like having
to
> convert a byte length to an element length and do the alignment check requires
> additional code, and I don't think there is clear winner in terms of which is
> the more readable code.
> 
> Is there a good reason not to go with the DOM names?
> byteOffset
> byteLength
> length
> bytesPerElement (BYTES_PER_ELEMENT)
> 
> The new names are longer but don't really add much: 
> offsetInBytes
> lengthInBytes
> length
> elementSizeInBytes
> 
> One advantage of keeping the same names for members is that the name in
> exception messages are closer to the source.  If we inline Dart
> "x.offsetInBytes" to  JS "x.byteOffset", and x is null, the message will
contain
> 'byteOffset', not 'offsetInBytes'.  It would be less confusing to debug the JS
> if they were both 'byteOffset'.
> 
>

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:483: [int start = 0, int length]);
renamed start to offsetInBytes.
length is not lengthInBytes so it remains length.

On 2013/02/26 07:25:30, sra1 wrote:
> Other things being equal, a constructor for an object with properties 'a' and
> 'b' where the values of 'a' and 'b' are determined by parameters to the
> constructor should have parameters named 'a' and 'b'.
> 
> If we stick with the current getter names, rename this to 
> 
> [int offsetInBytes = 0, int lengthInBytes]

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:484: }
I have added a BYTES_PER_ELEMENT constant in each of the List classes.

On 2013/02/26 07:25:30, sra1 wrote:
> The JS class/constructor, Int16Array, has a static constant BYTES_PER_ELEMENT,
> which can be used to divide a byteLength to get an element length.
> 
> Are we going to provide something similar to enable expressions like the
> following?
> 
> length = byteLength ~/ Int16List.BYTES_PER_ELEMENT;
> byteLength = length * Int16List.BYTES_PER_ELEMENT;

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:569: external factory
Uint32List.view(ByteBuffer buffer,
In this first cut I have left out the transferable stuff in order to get an
initial version implemented. The transferable stuff can be added once we are
done with this set.

On 2013/02/25 20:11:20, Anton Muhin wrote:
> there are no .transferable constructors, is it by design?

[sra1, 2013-02-28 02:41:58]

DBC #2

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:26: * Returns the number of bytes in the
representation of each element in this list.
Line length?

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:485: * the length of this object.
this object -> [buffer]

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:621: * more space- and time-efficient than
the default [List] implementation.
I agree that this boilerplate comment is true for Uint8List, but it might not be
true if the list contains values that predominantly fit in a Smi, especially on
a 32 bit VM.

perhaps 'may be' rather than 'will be'.

[Ivan Posva, 2013-02-28 05:00:01]

Next round of comments.

-Ivan

https://codereview.chromium.org/12313088/diff/19001/runtime/lib/typeddata.dart
File runtime/lib/typeddata.dart (right):

https://codereview.chromium.org/12313088/diff/19001/runtime/lib/typeddata.dar...
runtime/lib/typeddata.dart:2075: class _ByteDataIterator<E> implements
Iterator<E> {
I find it a bit confusing that the _ByteDataIterator is not used in the ByteData
class. Would it make sense to rename this to _TypedListIterator?

https://codereview.chromium.org/12313088/diff/19001/runtime/lib/typeddata.dar...
runtime/lib/typeddata.dart:2104: // and just implementing Collection<int>.
Please remove the comment and the "extends Collection<int>" otherwise we end up
having _Float32ListView extending Collection<int> and implementing List<double>,
which is a Collection<double> => my head explodes due to massive confusion.

https://codereview.chromium.org/12313088/diff/19001/runtime/lib/typeddata.dar...
runtime/lib/typeddata.dart:2107: : _typeddata = _buffer as TypedData,
What is the as used for here?

https://codereview.chromium.org/12313088/diff/19001/runtime/lib/typeddata.dar...
runtime/lib/typeddata.dart:2986: if (start is! int) throw new
ArgumentError("start is not an int");
No manual type checking!
Actually see my other comment about just dumping this method.

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
File sdk/lib/typeddata/typeddata.dart (right):

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata.dart:10: // regular library files.
This comment is not needed as it is out-of-date.

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata_base.dart:59: * is represented by the bytes of a
32-bit floating point number:
As far as I remember the plan was to allow ByteData unaligned access as we are
potentially dealing with files or network streams.

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata_base.dart:101: ByteData subByteData(int startIndex,
[int endIndex]);
Basically this is a sugar for:
var sb = new ByteData.view(original.buffer, original.offsetInBytes + startIndex,
endIndex-startIndex);

The startIndex, endIndex names do not match the rest of the names used in this
file. I am not sure this is really needed, how about removing it for now?

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata_base.dart:119: * Returns `byteOffset + 1`, which is
the offset of the first byte in the
I am not sure I understand the purpose of the return value of the setter.

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata_base.dart:371: * For long lists, this implementation
will be considerably
will -> can

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata_base.dart:488: [int offsetInBytes = 0, int length]);
Isn't the expectation that offsetInBytes is aligned to BYTES_PER_ELEMENT?

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata_base.dart:652: * numbers  that is viewable as a
[ByteArray]. For long lists, this
ByteArray?

https://codereview.chromium.org/12313088/diff/19001/sdk/lib/typeddata/typedda...
sdk/lib/typeddata/typeddata_base.dart:684: * numbers  that is viewable as a
[ByteArray]. For long lists, this
ditto

[Lasse Reichstein Nielsen, 2013-02-28 09:05:13]

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/7019/sdk/lib/typeddata/t...
sdk/lib/typeddata/typeddata_base.dart:23: abstract class TypedData {
Do you have a link to this g+ discussion?

[siva, 2013-03-01 01:15:56]

https://chromiumcodereview.appspot.com/12313088/diff/19001/runtime/lib/typedd...
File runtime/lib/typeddata.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/19001/runtime/lib/typedd...
runtime/lib/typeddata.dart:2075: class _ByteDataIterator<E> implements
Iterator<E> {
Makes sense, renamed to _TypedListIterator.

On 2013/02/28 05:00:02, Ivan Posva wrote:
> I find it a bit confusing that the _ByteDataIterator is not used in the
ByteData
> class. Would it make sense to rename this to _TypedListIterator?

https://chromiumcodereview.appspot.com/12313088/diff/19001/runtime/lib/typedd...
runtime/lib/typeddata.dart:2104: // and just implementing Collection<int>.
We already have _TypedListBase which sets up all the Collection methods, so I
changed this to extend _TypedListBase and removed all the collection/list
methods from this class (were just duplicates).

On 2013/02/28 05:00:02, Ivan Posva wrote:
> Please remove the comment and the "extends Collection<int>" otherwise we end
up
> having _Float32ListView extending Collection<int> and implementing
List<double>,
> which is a Collection<double> => my head explodes due to massive confusion.

https://chromiumcodereview.appspot.com/12313088/diff/19001/runtime/lib/typedd...
runtime/lib/typeddata.dart:2107: : _typeddata = _buffer as TypedData,
As discussed offline it is meant primarily for the static checker. I removed it
added a comment that the assignment is dynamically sound.

On 2013/02/28 05:00:02, Ivan Posva wrote:
> What is the as used for here?

https://chromiumcodereview.appspot.com/12313088/diff/19001/runtime/lib/typedd...
runtime/lib/typeddata.dart:2986: if (start is! int) throw new
ArgumentError("start is not an int");
On 2013/02/28 05:00:02, Ivan Posva wrote:
> No manual type checking!
> Actually see my other comment about just dumping this method.

Done.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
File sdk/lib/typeddata/typeddata.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata.dart:10: // regular library files.
On 2013/02/28 05:00:02, Ivan Posva wrote:
> This comment is not needed as it is out-of-date.

Done.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
File sdk/lib/typeddata/typeddata_base.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:26: * Returns the number of bytes in the
representation of each element in this list.
Fixed.

On 2013/02/28 02:41:58, sra1 wrote:
> Line length?

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:59: * is represented by the bytes of a
32-bit floating point number:
I thought random-access also meant unaligned access.
I could make it more explicit by adding a note on unaligned access.

On 2013/02/28 05:00:02, Ivan Posva wrote:
> As far as I remember the plan was to allow ByteData unaligned access as we are
> potentially dealing with files or network streams.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:101: ByteData subByteData(int startIndex,
[int endIndex]);
Yes. It is basically sugar for that with the additional Range checks. I think it
was trying to mimic the subarray call in JS. I have removed it for now. We can
always add it back if needed.

On 2013/02/28 05:00:02, Ivan Posva wrote:
> Basically this is a sugar for:
> var sb = new ByteData.view(original.buffer, original.offsetInBytes +
startIndex,
> endIndex-startIndex);
> 
> The startIndex, endIndex names do not match the rest of the names used in this
> file. I am not sure this is really needed, how about removing it for now?

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:119: * Returns `byteOffset + 1`, which is
the offset of the first byte in the
I don't remember why this returns setInt8 (Carl at some point explained this but
I don't recall).
We should remove it for now and can revisit if needed.

On 2013/02/28 05:00:02, Ivan Posva wrote:
> I am not sure I understand the purpose of the return value of the setter.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:371: * For long lists, this implementation
will be considerably
On 2013/02/28 05:00:02, Ivan Posva wrote:
> will -> can

Done.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:485: * the length of this object.
On 2013/02/28 02:41:58, sra1 wrote:
> this object -> [buffer]

Done.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:488: [int offsetInBytes = 0, int length]);
The current scalarlist implementation doesn't seem to require that but the JS
specification does state that the byteOffset should be a multiple of the element
size of the specific type. We should probably add that as an ArgumentError.

On 2013/02/28 05:00:02, Ivan Posva wrote:
> Isn't the expectation that offsetInBytes is aligned to BYTES_PER_ELEMENT?

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:621: * more space- and time-efficient than
the default [List] implementation.
The comment was changed to 'can be'.

Your point about a list consisting of predominantly Smi values being more
efficient than Int64List could be true if you stayed primarily in Dart, the
minute you start any DOM interaction and going across the C++ boundary the new
direct access API functions we have will definitely make Int64List more
efficient.

On 2013/02/28 02:41:58, sra1 wrote:
> I agree that this boilerplate comment is true for Uint8List, but it might not
be
> true if the list contains values that predominantly fit in a Smi, especially
on
> a 32 bit VM.
> 
> perhaps 'may be' rather than 'will be'.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:652: * numbers  that is viewable as a
[ByteArray]. For long lists, this
On 2013/02/28 05:00:02, Ivan Posva wrote:
> ByteArray?

Done.

https://chromiumcodereview.appspot.com/12313088/diff/19001/sdk/lib/typeddata/...
sdk/lib/typeddata/typeddata_base.dart:684: * numbers  that is viewable as a
[ByteArray]. For long lists, this
On 2013/02/28 05:00:02, Ivan Posva wrote:
> ditto

Done.

[Ivan Posva, 2013-03-04 18:47:52]

LGTM with small comments.

-Ivan

https://codereview.chromium.org/12313088/diff/20002/runtime/lib/typeddata.dart
File runtime/lib/typeddata.dart (right):

https://codereview.chromium.org/12313088/diff/20002/runtime/lib/typeddata.dar...
runtime/lib/typeddata.dart:275: dynamic reduce(dynamic initialValue,
dynamic is implied.

https://codereview.chromium.org/12313088/diff/20002/runtime/lib/typeddata.dar...
runtime/lib/typeddata.dart:445: implements TypedData, ByteBuffer {
I don't think TypedData is necessary here.

[siva, 2013-03-04 21:27:11]

https://chromiumcodereview.appspot.com/12313088/diff/20002/runtime/lib/typedd...
File runtime/lib/typeddata.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/20002/runtime/lib/typedd...
runtime/lib/typeddata.dart:275: dynamic reduce(dynamic initialValue,
This is following the style used in collections.dart.

It looks like there are two school of thoughts on how this should be coded. I am
going to leave it like this for now. We can maybe discuss with the library folks
and then uniformly change it everywhere.

On 2013/03/04 18:47:52, Ivan Posva wrote:
> dynamic is implied.

https://chromiumcodereview.appspot.com/12313088/diff/20002/runtime/lib/typedd...
runtime/lib/typeddata.dart:445: implements TypedData, ByteBuffer {
Yes, removed TypedData.

On 2013/03/04 18:47:52, Ivan Posva wrote:
> I don't think TypedData is necessary here.

[siva, 2013-03-04 22:11:59]

Committed patchset #11 manually as r19427 (presubmit successful).

[Lasse Reichstein Nielsen, 2013-03-05 12:02:02]

https://chromiumcodereview.appspot.com/12313088/diff/20002/runtime/lib/typedd...
File runtime/lib/typeddata.dart (right):

https://chromiumcodereview.appspot.com/12313088/diff/20002/runtime/lib/typedd...
runtime/lib/typeddata.dart:275: dynamic reduce(dynamic initialValue,
For parameters, I'd prefer "var" to "dynamic". It means exactly that same, and I
don't want to omit it completely (which would also mean the same).
For return types, we do use "dynamic" because, again, I don't like omitting it
entirely - that just looks like someone forgot to add a type annotation, not
someone deliberately picking dynamic.
That is the signature in Iterable: 
 dynamic reduce(var initialValue, dynamic combine(var initialValue, E element))

[floitsch, 2013-03-05 14:18:46]

On 2013/03/05 12:02:02, Lasse Reichstein Nielsen wrote:
>
https://chromiumcodereview.appspot.com/12313088/diff/20002/runtime/lib/typedd...
> File runtime/lib/typeddata.dart (right):
> 
>
https://chromiumcodereview.appspot.com/12313088/diff/20002/runtime/lib/typedd...
> runtime/lib/typeddata.dart:275: dynamic reduce(dynamic initialValue,
> For parameters, I'd prefer "var" to "dynamic". It means exactly that same, and
I
> don't want to omit it completely (which would also mean the same).
> For return types, we do use "dynamic" because, again, I don't like omitting it
> entirely - that just looks like someone forgot to add a type annotation, not
> someone deliberately picking dynamic.
> That is the signature in Iterable: 
>  dynamic reduce(var initialValue, dynamic combine(var initialValue, E
element))

The "dynamic" came because of me (I suggested it yesterday). Clearly we don't
have a common guideline yet (mostly because there aren't many use-cases for it).

I use "dynamic" to state that I have thought about the type but that the input
can be anything. "var" to say "I just don't want to write one" ("var x = new
Foo()").
And, as Lasse states, "dynamic" is the only way to write return types that can
be anything.

[Florian Schneider, 2013-03-12 14:42:27]

https://codereview.chromium.org/12313088/diff/35002/runtime/lib/typeddata.dart
File runtime/lib/typeddata.dart (right):

https://codereview.chromium.org/12313088/diff/35002/runtime/lib/typeddata.dar...
runtime/lib/typeddata.dart:2918: _typedata._setUint32(_offset + byteOffset,
value);

s/_typedata/_typeddata/g

here and below.  This seems to be not covered by tests?