Use ByteArray's native for Socket and File.

Use ByteArray's native for Socket and File.
Committed: https://code.google.com/p/dart/source/detail?r=3812

[Anders Johnsen, 2012-01-26 19:38:13]

Utilize the new ByteArray for Socket and File. This also makes the API methods
Dart_ListGetAsBytes, Dart_ListSetAsBytes and Dart_ListLength handle ByteArrays
natively.

https://chromiumcodereview.appspot.com/9235067/diff/1/runtime/lib/byte_array....
File runtime/lib/byte_array.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/1/runtime/lib/byte_array....
runtime/lib/byte_array.dart:86: static copy(ByteArray dst,
I'm not entirely happy with the placement of this and the name of the native
method. Any comments are welcome!

[cshapiro, 2012-01-27 23:00:04]

http://codereview.chromium.org/9235067/diff/1/runtime/bin/socket_impl.dart
File runtime/bin/socket_impl.dart (right):

http://codereview.chromium.org/9235067/diff/1/runtime/bin/socket_impl.dart#ne...
runtime/bin/socket_impl.dart:312: List<int> outBuffer;
I think this can safely be retyped as a ByteArray even if the enclosing method
typed to return a List<int>, similar to how ObjectArray is used now.

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.cc
File runtime/lib/byte_array.cc (right):

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:39: intptr_t icount = count.Value();
I think the Value() calls are compiled away so you do not need to do the
subexpression elimination on your own.

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:86: static copy(ByteArray dst,
I would create a private instance method _setRange which calls a native
ByteArray_setRange.  The term copy seems ambiguous in this context, especially
because it does not return a copy but rather copies into something.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9235067/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:1362: if (obj.IsByteArray()) {
Two questions.  Why is this ahead of array and why is this now in native code in
the first place?  Same goes for the code below.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:1580: return Api::NewError("Invalid length passed in
to access array elements");
list elements, right?  I think the list interface was the array interface
initially.  From the user point of view, it is still a list, even though the
internals have not been renamed to reflect the change.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc#newcode7519
runtime/vm/object.cc:7519: const uint8_t *src_addr =
src.ByteAddr(src_byte_offset);
This should be wrapped in a NoGCScope.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc#newcode7529
runtime/vm/object.cc:7529: uint8_t *dst_addr = dst.ByteAddr(dst_byte_offset);
Same here...

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc#newcode7542
runtime/vm/object.cc:7542: uint8_t *dst_addr = dst.ByteAddr(dst_byte_offset);
...and here.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.h#newcode3255
runtime/vm/object.h:3255: intptr_t src_byte_offset,
You can probably shorten src_byte_offset to just src_offset here and elsewhere
as the type is implied.  I used the term byte offset to make it clear that even
with the template types, the index was always in terms of bytes and not scaled
byte the template argument.  It's pretty clear here.

[Anders Johnsen, 2012-01-27 23:42:39]

Let me know if you want me to move the ByteArray-case down below the ArrayCase.
Thanks for the review!

http://codereview.chromium.org/9235067/diff/1/runtime/bin/socket_impl.dart
File runtime/bin/socket_impl.dart (right):

http://codereview.chromium.org/9235067/diff/1/runtime/bin/socket_impl.dart#ne...
runtime/bin/socket_impl.dart:312: List<int> outBuffer;
On 2012/01/27 23:00:04, cshapiro wrote:
> I think this can safely be retyped as a ByteArray even if the enclosing method
> typed to return a List<int>, similar to how ObjectArray is used now.

The thing is, that both a ByteArray and an ObjectArray can be assigned to
outBuffer, List<int> being the common ancestor.

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.cc
File runtime/lib/byte_array.cc (right):

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:39: intptr_t icount = count.Value();
On 2012/01/27 23:00:04, cshapiro wrote:
> I think the Value() calls are compiled away so you do not need to do the
> subexpression elimination on your own.

Done.

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:86: static copy(ByteArray dst,
On 2012/01/27 23:00:04, cshapiro wrote:
> I would create a private instance method _setRange which calls a native
> ByteArray_setRange.  The term copy seems ambiguous in this context, especially
> because it does not return a copy but rather copies into something.

Thank you, way better! :)

http://codereview.chromium.org/9235067/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9235067/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:1362: if (obj.IsByteArray()) {
On 2012/01/27 23:00:04, cshapiro wrote:
> Two questions.  Why is this ahead of array and why is this now in native code
in
> the first place?  Same goes for the code below.

Adding the native operations for getting/setting data and getting the length in
these methods, will make ByteArray's work in File/Socket/etc. without further
modifications. I placed it here since I see it as a more specialized case of
Array (even though that might not be the case). I will move it down if you still
feel like?

http://codereview.chromium.org/9235067/diff/1/runtime/vm/dart_api_impl.cc#new...
runtime/vm/dart_api_impl.cc:1580: return Api::NewError("Invalid length passed in
to access array elements");
On 2012/01/27 23:00:04, cshapiro wrote:
> list elements, right?  I think the list interface was the array interface
> initially.  From the user point of view, it is still a list, even though the
> internals have not been renamed to reflect the change.

Done.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc#newcode7519
runtime/vm/object.cc:7519: const uint8_t *src_addr =
src.ByteAddr(src_byte_offset);
On 2012/01/27 23:00:04, cshapiro wrote:
> This should be wrapped in a NoGCScope.

Done.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc#newcode7529
runtime/vm/object.cc:7529: uint8_t *dst_addr = dst.ByteAddr(dst_byte_offset);
On 2012/01/27 23:00:04, cshapiro wrote:
> Same here...

Done.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.cc#newcode7542
runtime/vm/object.cc:7542: uint8_t *dst_addr = dst.ByteAddr(dst_byte_offset);
On 2012/01/27 23:00:04, cshapiro wrote:
> ...and here.

Done.

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/9235067/diff/1/runtime/vm/object.h#newcode3255
runtime/vm/object.h:3255: intptr_t src_byte_offset,
On 2012/01/27 23:00:04, cshapiro wrote:
> You can probably shorten src_byte_offset to just src_offset here and elsewhere
> as the type is implied.  I used the term byte offset to make it clear that
even
> with the template types, the index was always in terms of bytes and not scaled
> byte the template argument.  It's pretty clear here.

Done.

[cshapiro, 2012-01-28 02:25:48]

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:22: List<int> result = new ByteArray(bytesToRead);
This can be a ByteArray now.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:25: List<int> buffer = new ByteArray(bytesRead);
Same here.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:178: var buffer = new ByteArray(_bytes);
No need for the unspecific type here.  (I don't think there ever was.)

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:424: // When using the Dart C API access to
ObjectArray by index is
Update this comment.  Improvements to the English are welcome.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_impl.dart
File runtime/bin/socket_impl.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_impl.dart...
runtime/bin/socket_impl.dart:309: // When using the Dart C API access to
ObjectArray by index is
Same comments in file_impl.dart apply here.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_stream_im...
File runtime/bin/socket_stream_impl.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_stream_im...
runtime/bin/socket_stream_impl.dart:20: List<int> buffer = new
ByteArray(bytesToRead);
This can be typed as a byte array in all cases.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc
File runtime/lib/byte_array.cc (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:43: if (count.Value() == 0) {
technically, this is not needed. your call.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:46: if ((src_start.Value() < 0) ||
This check must be improved.  Ensure all of the following or throw.

src_start.Value() >= 0
count.Value() >= 0
count.Value() <= (src.Length() - src_start.Value())

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:52: if ((dst_start.Value() < 0) ||
Same here.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:65: 
This code should be moved downward beneath the List interface header.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:85: void _setRange(ByteArray src,
This should be moved to the very bottom with the other native (implementation)
methods.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:679: result = Dart_ListSetAt(obj1, i,
Dart_NewInteger(10 - i));
You can wrap the entire call in expect_valid to keep it short.  Your call.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:684: Dart_Handle e = Dart_ListGetAt(obj1, i);
Check this for validity, too.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:7518: ASSERT(src_offset + length <= src.Length());
This assert is insufficient... for example

  ASSERT(dst_offset >= 0);
  ASSERT(src_offset >= 0);
  ASSERT(len >= 0);
  ASSERT(len <= (dst.Length() - dst_offset));
  ASSERT(len <= (src.Length() - src_offset));

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:7530: ASSERT(dst_offset + length <= dst.Length());
Same here.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:7543: ASSERT(src_offset + length <= src.Length());
Same here.

[Anders Johnsen, 2012-01-30 21:15:28]

Thank you for the review, spotted some very good corners! I've fixed the cases
in question, added a new Utils::RangeCheck to handle the range checks. If you
approve this approach, I'll update other methods (String::Copy) to use it in a
different cl.

I've also added a check in operator= for ByteArray, to make sure it's an integer
in byte range.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:22: List<int> result = new ByteArray(bytesToRead);
On 2012/01/28 02:25:48, cshapiro wrote:
> This can be a ByteArray now.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:25: List<int> buffer = new ByteArray(bytesRead);
On 2012/01/28 02:25:48, cshapiro wrote:
> Same here.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:178: var buffer = new ByteArray(_bytes);
On 2012/01/28 02:25:48, cshapiro wrote:
> No need for the unspecific type here.  (I don't think there ever was.)

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/file_impl.dart#n...
runtime/bin/file_impl.dart:424: // When using the Dart C API access to
ObjectArray by index is
On 2012/01/28 02:25:48, cshapiro wrote:
> Update this comment.  Improvements to the English are welcome.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_impl.dart
File runtime/bin/socket_impl.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_impl.dart...
runtime/bin/socket_impl.dart:309: // When using the Dart C API access to
ObjectArray by index is
On 2012/01/28 02:25:48, cshapiro wrote:
> Same comments in file_impl.dart apply here.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_stream_im...
File runtime/bin/socket_stream_impl.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/bin/socket_stream_im...
runtime/bin/socket_stream_impl.dart:20: List<int> buffer = new
ByteArray(bytesToRead);
On 2012/01/28 02:25:48, cshapiro wrote:
> This can be typed as a byte array in all cases.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc
File runtime/lib/byte_array.cc (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:43: if (count.Value() == 0) {
On 2012/01/28 02:25:48, cshapiro wrote:
> technically, this is not needed. your call.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:46: if ((src_start.Value() < 0) ||
On 2012/01/28 02:25:48, cshapiro wrote:
> This check must be improved.  Ensure all of the following or throw.
> 
> src_start.Value() >= 0
> count.Value() >= 0
> count.Value() <= (src.Length() - src_start.Value())

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:52: if ((dst_start.Value() < 0) ||
On 2012/01/28 02:25:48, cshapiro wrote:
> Same here.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:65: 
On 2012/01/28 02:25:48, cshapiro wrote:
> This code should be moved downward beneath the List interface header.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:85: void _setRange(ByteArray src,
On 2012/01/28 02:25:48, cshapiro wrote:
> This should be moved to the very bottom with the other native (implementation)
> methods.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:679: result = Dart_ListSetAt(obj1, i,
Dart_NewInteger(10 - i));
On 2012/01/28 02:25:48, cshapiro wrote:
> You can wrap the entire call in expect_valid to keep it short.  Your call.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:684: Dart_Handle e = Dart_ListGetAt(obj1, i);
On 2012/01/28 02:25:48, cshapiro wrote:
> Check this for validity, too.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:7518: ASSERT(src_offset + length <= src.Length());
On 2012/01/28 02:25:48, cshapiro wrote:
> This assert is insufficient... for example
> 
>   ASSERT(dst_offset >= 0);
>   ASSERT(src_offset >= 0);
>   ASSERT(len >= 0);
>   ASSERT(len <= (dst.Length() - dst_offset));
>   ASSERT(len <= (src.Length() - src_offset));

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:7530: ASSERT(dst_offset + length <= dst.Length());
On 2012/01/28 02:25:48, cshapiro wrote:
> Same here.

Done.

http://codereview.chromium.org/9235067/diff/9001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:7543: ASSERT(src_offset + length <= src.Length());
On 2012/01/28 02:25:48, cshapiro wrote:
> Same here.

Done.

[Anders Johnsen, 2012-01-30 21:30:48]

Spotted a very bad bug! Fixed and uploaded.

[cshapiro, 2012-01-31 03:04:29]

lgtm

http://codereview.chromium.org/9235067/diff/16002/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:99: if (value is! int || !(value >= 0 && value <=
255)) {
Why not simplify this expression?

  if ((value is int) && (value >= 0) && (value <= 256)) {
    // set
  } else {
    // throw
  }

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1576: if ((offset + length) <= byte_array.Length())
{
This looks like another opportunity for integer overflow.  It will have to be
cleaned up here and elsewhere.  We should probably file a bug and do a cleanup
in a future change.

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1648: if ((offset + length) <= byte_array.Length())
{
Ditto.

[cshapiro, 2012-01-31 03:12:03]

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7544: ASSERT(Utils::RangeCheck(dst_offset, length,
dst.Length()));
This function does not yet seem to exist in Utils.  If you are going to add it,
why not eliminate the function of the same name in byte_array.cc?

[Anders Johnsen, 2012-01-31 03:34:56]

Updated with utils.h. It would be great if you could give a quick look
(utils.h), and I'll do a commit after that. Thanks for the review!

http://codereview.chromium.org/9235067/diff/16002/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:99: if (value is! int || !(value >= 0 && value <=
255)) {
On 2012/01/31 03:04:29, cshapiro wrote:
> Why not simplify this expression?
> 
>   if ((value is int) && (value >= 0) && (value <= 256)) {
>     // set
>   } else {
>     // throw
>   }
> 

Done.

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1576: if ((offset + length) <= byte_array.Length())
{
On 2012/01/31 03:04:29, cshapiro wrote:
> This looks like another opportunity for integer overflow.  It will have to be
> cleaned up here and elsewhere.  We should probably file a bug and do a cleanup
> in a future change.

Done.

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:1648: if ((offset + length) <= byte_array.Length())
{
On 2012/01/31 03:04:29, cshapiro wrote:
> Ditto.

Done.

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9235067/diff/16002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:7544: ASSERT(Utils::RangeCheck(dst_offset, length,
dst.Length()));
On 2012/01/31 03:12:03, cshapiro wrote:
> This function does not yet seem to exist in Utils.  If you are going to add
it,
> why not eliminate the function of the same name in byte_array.cc?

Sorry, forgot to add file to cl. Added now.

[Anders Johnsen, 2012-01-31 20:18:40]

Added Ivan as review.

[Anders Johnsen, 2012-01-31 20:41:17]

Kept the local RangeCheck in byte_array.cc, but uses it a few more places. It's
a nice wrapper for a common (local) case.

[Ivan Posva, 2012-02-01 01:05:27]

http://codereview.chromium.org/9235067/diff/23001/runtime/lib/byte_array.cc
File runtime/lib/byte_array.cc (right):

http://codereview.chromium.org/9235067/diff/23001/runtime/lib/byte_array.cc#n...
runtime/lib/byte_array.cc:33: static void RangeCheck(const ByteArray& array,
const Smi& index,
Why not just use intptr_t as parameters here?

http://codereview.chromium.org/9235067/diff/23001/runtime/lib/byte_array.cc#n...
runtime/lib/byte_array.cc:48: GET_NATIVE_ARGUMENT(Smi, count, arguments->At(4));
intptr_t count_value = count.Value();
and friends...

http://codereview.chromium.org/9235067/diff/23001/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/23001/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:99: if (value is int && value >= 0 && value <= 255)
{
Why this semantic change?

http://codereview.chromium.org/9235067/diff/23001/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:173: void _setRange(ByteArray src,
It is very confusing to have two methods with essentially the same name have
different argument lists. Please fix this before committing.

You should either add a private static method _copy which mirrors the
Arrays.copy method or mirror the setRange arguments here, without the
optionality of startFrom as it would not be needed.

[Ivan Posva, 2012-02-01 01:12:41]

More comments.

-Ivan

http://codereview.chromium.org/9235067/diff/23001/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9235067/diff/23001/runtime/bin/file_impl.dart#...
runtime/bin/file_impl.dart:431: } else {
How about not copying ObjectArrays either? They do have reasonably fast handling
in native code as well, and you will avoid making extra copies.

http://codereview.chromium.org/9235067/diff/23001/runtime/bin/socket_impl.dart
File runtime/bin/socket_impl.dart (right):

http://codereview.chromium.org/9235067/diff/23001/runtime/bin/socket_impl.dar...
runtime/bin/socket_impl.dart:316: } else {
ditto

http://codereview.chromium.org/9235067/diff/23001/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/9235067/diff/23001/runtime/vm/object.h#newcode...
runtime/vm/object.h:3252: static void Copy(uint8_t* dst,
The inconsistency of arguments in these three functions is very confusing.

[Anders Johnsen, 2012-02-01 02:12:10]

Thank you for the feedback! Since I get some minor contradictions, I've left
several of the topics open for further comments.

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/file_im...
runtime/bin/file_impl.dart:431: } else {
On 2012/02/01 01:12:41, Ivan Posva wrote:
> How about not copying ObjectArrays either? They do have reasonably fast
handling
> in native code as well, and you will avoid making extra copies.

That was the initial idea, but Carl and I were thinking about moving in the
direction of the native writeListNative method accepting only ByteArrays (and
reflect that in c++ code down the hill). Likewise for socket. If you disagree
about that, I'll re-add the ObjectArray case.

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/socket_...
File runtime/bin/socket_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/socket_...
runtime/bin/socket_impl.dart:316: } else {
On 2012/02/01 01:12:41, Ivan Posva wrote:
> ditto

(see comment to file_impl.dart)

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
File runtime/lib/byte_array.cc (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
runtime/lib/byte_array.cc:33: static void RangeCheck(const ByteArray& array,
const Smi& index,
On 2012/02/01 01:05:27, Ivan Posva wrote:
> Why not just use intptr_t as parameters here?

We use the Smi object to cast the error (line 37).

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
runtime/lib/byte_array.cc:48: GET_NATIVE_ARGUMENT(Smi, count, arguments->At(4));
On 2012/02/01 01:05:27, Ivan Posva wrote:
> intptr_t count_value = count.Value();
> and friends...

Emm okay, I'm getting some conflicting comments on this one. I'll update to
using temporary (readability), but I'd like to know what the general rule is
here.

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
File runtime/lib/byte_array.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
runtime/lib/byte_array.dart:99: if (value is int && value >= 0 && value <= 255)
{
On 2012/02/01 01:05:27, Ivan Posva wrote:
> Why this semantic change?

Are you meaning why to move the 

if(!...) throw; setUint8 

into

if(...) setUint8 else throw;

it's due to readability (if I interpreted Carl's feedback correctly). If not,
please specify. :)

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
runtime/lib/byte_array.dart:173: void _setRange(ByteArray src,
On 2012/02/01 01:05:27, Ivan Posva wrote:
> It is very confusing to have two methods with essentially the same name have
> different argument lists. Please fix this before committing.
> 
> You should either add a private static method _copy which mirrors the
> Arrays.copy method or mirror the setRange arguments here, without the
> optionality of startFrom as it would not be needed.

I absolutely agree, very good observation. I've update the argument order.

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/vm/object.h...
runtime/vm/object.h:3252: static void Copy(uint8_t* dst,
On 2012/02/01 01:12:41, Ivan Posva wrote:
> The inconsistency of arguments in these three functions is very confusing.

If you refer to the "lack" of either src_offset or dst_offset, I'm only partly
agreeing. My argument for this is, that if I'm presented with an uint8_t pointer
_and_ an offset, I have no way of performing a range check. So, to not make the
user make that assumptation, the methods only invariant is that the pointer is
at least the size of length. If you feel it's save to make that extra invariant,
let me know and I'll update it.

[Søren Gjesse, 2012-02-01 08:16:08]

dbc nits

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/file_im...
runtime/bin/file_impl.dart:424: // When using the Dart C API to access raw data,
using ByteArray is
"using ByteArray" -> "using a ByteArray"

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/file_im...
runtime/bin/file_impl.dart:440: "List element is not an integer in byte range");
"in byte range" -> "in the byte range"

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/socket_...
File runtime/bin/socket_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/socket_...
runtime/bin/socket_impl.dart:309: // When using the Dart C API to access raw
data, using ByteArray is
See comment file_impl.dart.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/socket_...
runtime/bin/socket_impl.dart:325: "List element is not an integer in byte
range");
See comment in file_impl.dart.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/lib/byte_ar...
File runtime/lib/byte_array.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/lib/byte_ar...
runtime/lib/byte_array.dart:103: "value is not an integer in byte range");
"in byte range" -> "in the byte range"

Start message with uppercase.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/lib/byte_ar...
runtime/lib/byte_array.dart:176: int startFrom)
I think the parameter list can fit on a single line.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/vm/dart_api...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/vm/dart_api...
runtime/vm/dart_api_impl_test.cc:617: Dart_Handle obj1 = Dart_NewByteArray(10);
Why not rename "obj1" to "byte_array"?

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/vm/dart_api...
runtime/vm/dart_api_impl_test.cc:689: }
Maybe add a test with list values outside the byte range.

[Anders Johnsen, 2012-02-01 18:54:53]

Thank you for the feedback Soeren! I've added a new test file that actually
exposed an error. I've added two new 'if (len > 0)' guards in object.cc, since
it otherwise would assert.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/file_im...
runtime/bin/file_impl.dart:424: // When using the Dart C API to access raw data,
using ByteArray is
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> "using ByteArray" -> "using a ByteArray"

Done.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/file_im...
runtime/bin/file_impl.dart:440: "List element is not an integer in byte range");
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> "in byte range" -> "in the byte range"

Done.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/socket_...
File runtime/bin/socket_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/socket_...
runtime/bin/socket_impl.dart:309: // When using the Dart C API to access raw
data, using ByteArray is
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> See comment file_impl.dart.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/bin/socket_...
runtime/bin/socket_impl.dart:325: "List element is not an integer in byte
range");
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> See comment in file_impl.dart.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/lib/byte_ar...
File runtime/lib/byte_array.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/lib/byte_ar...
runtime/lib/byte_array.dart:103: "value is not an integer in byte range");
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> "in byte range" -> "in the byte range"
> 
> Start message with uppercase.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/lib/byte_ar...
runtime/lib/byte_array.dart:176: int startFrom)
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> I think the parameter list can fit on a single line.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/vm/dart_api...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/vm/dart_api...
runtime/vm/dart_api_impl_test.cc:617: Dart_Handle obj1 = Dart_NewByteArray(10);
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> Why not rename "obj1" to "byte_array"?

Will fix!

https://chromiumcodereview.appspot.com/9235067/diff/28003/runtime/vm/dart_api...
runtime/vm/dart_api_impl_test.cc:689: }
On 2012/02/01 08:16:08, Søren Gjesse wrote:
> Maybe add a test with list values outside the byte range.

Added new file.

[Ivan Posva, 2012-02-01 20:01:34]

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/file_im...
runtime/bin/file_impl.dart:431: } else {
On 2012/02/01 02:12:11, ajohnsen wrote:
> On 2012/02/01 01:12:41, Ivan Posva wrote:
> > How about not copying ObjectArrays either? They do have reasonably fast
> handling
> > in native code as well, and you will avoid making extra copies.
> 
> That was the initial idea, but Carl and I were thinking about moving in the
> direction of the native writeListNative method accepting only ByteArrays (and
> reflect that in c++ code down the hill). Likewise for socket. If you disagree
> about that, I'll re-add the ObjectArray case.

I think all List implementations that are native to the VM should be acceptable
here.

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
File runtime/lib/byte_array.cc (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
runtime/lib/byte_array.cc:33: static void RangeCheck(const ByteArray& array,
const Smi& index,
On 2012/02/01 02:12:11, ajohnsen wrote:
> On 2012/02/01 01:05:27, Ivan Posva wrote:
> > Why not just use intptr_t as parameters here?
> 
> We use the Smi object to cast the error (line 37).

Create a new Integer handle when you really need to throw the exception.

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/file_im...
runtime/bin/file_impl.dart:440: "List element is not an integer in the byte
range");
Better diagnostics could include the offending index.

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/socket_...
File runtime/bin/socket_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/socket_...
runtime/bin/socket_impl.dart:325: "List element is not an integer in the byte
range");
ditto

[Anders Johnsen, 2012-02-01 20:20:41]

I've updated the code to reflect the lasts comments both on and off this list.
I've reverted some code and created this bug for future discussion of how to
utilize the ByteArray: http://code.google.com/p/dart/issues/detail?id=1463

Again, thank you for all the feedback!

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/bin/file_im...
runtime/bin/file_impl.dart:431: } else {
On 2012/02/01 20:01:34, Ivan Posva wrote:
> On 2012/02/01 02:12:11, ajohnsen wrote:
> > On 2012/02/01 01:12:41, Ivan Posva wrote:
> > > How about not copying ObjectArrays either? They do have reasonably fast
> > handling
> > > in native code as well, and you will avoid making extra copies.
> > 
> > That was the initial idea, but Carl and I were thinking about moving in the
> > direction of the native writeListNative method accepting only ByteArrays
(and
> > reflect that in c++ code down the hill). Likewise for socket. If you
disagree
> > about that, I'll re-add the ObjectArray case.
> 
> I think all List implementations that are native to the VM should be
acceptable
> here.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
File runtime/lib/byte_array.cc (right):

https://chromiumcodereview.appspot.com/9235067/diff/23001/runtime/lib/byte_ar...
runtime/lib/byte_array.cc:33: static void RangeCheck(const ByteArray& array,
const Smi& index,
On 2012/02/01 20:01:34, Ivan Posva wrote:
> On 2012/02/01 02:12:11, ajohnsen wrote:
> > On 2012/02/01 01:05:27, Ivan Posva wrote:
> > > Why not just use intptr_t as parameters here?
> > 
> > We use the Smi object to cast the error (line 37).
> 
> Create a new Integer handle when you really need to throw the exception.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/file_im...
runtime/bin/file_impl.dart:440: "List element is not an integer in the byte
range");
On 2012/02/01 20:01:34, Ivan Posva wrote:
> Better diagnostics could include the offending index.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/socket_...
File runtime/bin/socket_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22003/runtime/bin/socket_...
runtime/bin/socket_impl.dart:325: "List element is not an integer in the byte
range");
On 2012/02/01 20:01:34, Ivan Posva wrote:
> ditto

Done.

[Ivan Posva, 2012-02-01 21:28:57]

LGTM with comments.

-Ivan

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/file_im...
runtime/bin/file_impl.dart:429: if (buffer is ByteArray) {
if ((buffer is ByteArray) || (buffer is ObjectArray)) {

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/file_im...
runtime/bin/file_impl.dart:438: if (buffer[j] is! int) {
You might want to cache the buffer[j] value in a local.

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/socket_...
File runtime/bin/socket_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/socket_...
runtime/bin/socket_impl.dart:314: if (buffer is ByteArray) {
ditto

[Anders Johnsen, 2012-02-01 21:48:14]

Fixed, and committed. Thank you all for the reviews. I've learned a lot here!

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/file_im...
runtime/bin/file_impl.dart:429: if (buffer is ByteArray) {
On 2012/02/01 21:28:57, Ivan Posva wrote:
> if ((buffer is ByteArray) || (buffer is ObjectArray)) {

Done.

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/file_im...
runtime/bin/file_impl.dart:438: if (buffer[j] is! int) {
On 2012/02/01 21:28:57, Ivan Posva wrote:
> You might want to cache the buffer[j] value in a local.

Done.

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/socket_...
File runtime/bin/socket_impl.dart (right):

https://chromiumcodereview.appspot.com/9235067/diff/22004/runtime/bin/socket_...
runtime/bin/socket_impl.dart:314: if (buffer is ByteArray) {
On 2012/02/01 21:28:57, Ivan Posva wrote:
> ditto

Done.

[jat, 2012-02-01 23:09:29]

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:5: interface ByteArray extends List default
_InternalByteArray {
How will this interoperate with WebGL buffers?  Can an instance of this be
passed directly, or will it have to be copied?  Likewise with XHR/WebSocket.

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:9: 
Should there be some method for encoding/decoding arbitrary precision integers?

[Anders Johnsen, 2012-02-01 23:19:30]

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:5: interface ByteArray extends List default
_InternalByteArray {
On 2012/02/01 23:09:29, jat wrote:
> How will this interoperate with WebGL buffers?  Can an instance of this be
> passed directly, or will it have to be copied?  Likewise with XHR/WebSocket.

I believe that an ExternalByteArray could be used for such buffers. It will be
able to encapsulate any provided byte-buffer, without the need for copying.
Depending on how you want to handle WebSocket, InternalByteArray may be just
fine. Maybe Carl may have some better answers here.

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:9: 
On 2012/02/01 23:09:29, jat wrote:
> Should there be some method for encoding/decoding arbitrary precision
integers?

One could create a class to operate on top of a ByteArray, to do such
operations. I'm unsure if the responsibility should be placed here.

[Ivan Posva, 2012-02-02 08:26:42]

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9235067/diff/28035/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:5: interface ByteArray extends List default
_InternalByteArray {
On 2012/02/01 23:09:29, jat wrote:
> How will this interoperate with WebGL buffers?  Can an instance of this be
> passed directly, or will it have to be copied?  Likewise with XHR/WebSocket.

The plan was that if an internal ByteArray is handed to the WebGL bindings, then
we do either copy the contents or convert it to an external ByteArray (for
reference please see how the V8 bindings can convert heap allocated strings to
external strings to avoid further copies).

Handing out raw pointers into garbage collected heaps to graphics cards
guarantees crashes, long debugging sessions and I will get another plaque.