Add an external array type to support typed arrays.

Add an external array type to support typed arrays.

Committed: https://code.google.com/p/dart/source/detail?r=1601

[siva, 2011-11-02 17:05:30]

LGTM once comments regarding byte_start and byte_end is addressed.

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/object.h#newcode2856
runtime/vm/object.h:2856: intptr_t byte_start = index / sizeof(T);
is this right:

for instance Addr<int32_t>(1) should produce an address
of &(raw_ptr()->data_[4]); which would be index * sizeof(T)

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/object_test.cc#newco...
runtime/vm/object_test.cc:1615: EXPECT_EQ(2, array1.Uint8At(5));
We probably need unit tests for get/set of other types too, maybe have a
templatized version or something to avoid having to retype this again.

http://codereview.chromium.org/8429027/diff/1/runtime/vm/object_test.cc#newco...
runtime/vm/object_test.cc:1626: EXPECT(array2.Equals(array1));
We should set some different values and check that array1.Equals(array2) returns
false in that case.

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/raw_object.cc#newcod...
runtime/vm/raw_object.cc:482: ASSERT(raw_obj->IsHeapObject());
I would add
visitor->VisitPointers(from, to) for consistency.

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/raw_object.h#newcode726
runtime/vm/raw_object.h:726: RawSmi* length_;
Do we want this to be RawSmi or intptr_t i.e have a restriction on how big the
external array can be or let it be larger than what Smi can hold?

Also missing from() and to() methods, even though you might have only a RawSmi
in here it is probably a good idea to add the from and to so that it is
consistent with the others and if somebody decides to add a field in here later
it is not overlooked.

[cshapiro, 2011-11-04 18:16:21]

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/object.h#newcode2856
runtime/vm/object.h:2856: intptr_t byte_start = index / sizeof(T);
No, it was not.  I fixed this in the more recent patch set.

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/object_test.cc#newco...
runtime/vm/object_test.cc:1615: EXPECT_EQ(2, array1.Uint8At(5));
It is not entirely clear how to do this.  I have simplified the access routine
so the getter and setter is now templatized although I think you are suggesting
a more meaningful change.

http://codereview.chromium.org/8429027/diff/1/runtime/vm/object_test.cc#newco...
runtime/vm/object_test.cc:1626: EXPECT(array2.Equals(array1));
Absolutely.  Done in the more recent patch set.

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/raw_object.cc#newcod...
runtime/vm/raw_object.cc:482: ASSERT(raw_obj->IsHeapObject());
Added. Done.

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

http://codereview.chromium.org/8429027/diff/1/runtime/vm/raw_object.h#newcode726
runtime/vm/raw_object.h:726: RawSmi* length_;
I am partial to having the length parameter to be a Smi as the type should
implement List and Collection and the extant implementations of these types are
limit to Smi indices.  I am not sure how to reconcile this disparity in reach. 
What do you think?

As for the from and to methods, I will go ahead add that.

[cshapiro, 2011-11-10 00:23:47]

PTAL

[Ivan Posva, 2011-11-10 18:06:41]

Older comments were stuck. Not sure whether they still apply.

-Ivan

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.cc
File runtime/lib/byte_buffer.cc (right):

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:25: if (length.IsNull() || (length.Value() < 0)) {
Shouldn't length.IsNull already be caught by the IsSmi() check above?

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.dart
File runtime/lib/byte_buffer.dart (right):

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.dar...
runtime/lib/byte_buffer.dart:6: factory ByteBuffer(int length) native
"ByteBuffer_allocate";
Please make all natives be private methods that are called from the exposed API.

[Ivan Posva, 2011-11-10 22:31:02]

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc
File runtime/lib/byte_buffer.cc (right):

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:65: if ((index.Value() < 0) || (index.Value() >=
buffer.Length())) {
You can still potentially read past the end of the buffer, no? For example you
have a 3 element byte buffer, and read a int32 from offset 0.

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:71:
ObjectType::Handle(ObjectType::New(buffer.At<ValueType>(index.Value()))));
Why are you assuming aligned addresses here and not in the set case?

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:128: if ((index.Value() < 0) || (index.Value() >=
buffer.Length())) {
ditto

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/globals.h
File runtime/vm/globals.h (right):

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/globals.h#newcode36
runtime/vm/globals.h:36: #include <float.h>
What is this used for?

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:727: RawObject** to() {
One line? Same as from() above?

[cshapiro, 2011-11-11 00:37:53]

PTAL

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.cc
File runtime/lib/byte_buffer.cc (right):

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:25: if (length.IsNull() || (length.Value() < 0)) {
I suppose so.  Fixed.

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.dart
File runtime/lib/byte_buffer.dart (right):

http://codereview.chromium.org/8429027/diff/11001/runtime/lib/byte_buffer.dar...
runtime/lib/byte_buffer.dart:6: factory ByteBuffer(int length) native
"ByteBuffer_allocate";
Done.

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc
File runtime/lib/byte_buffer.cc (right):

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:65: if ((index.Value() < 0) || (index.Value() >=
buffer.Length())) {
Yes.  Fixed.

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:71:
ObjectType::Handle(ObjectType::New(buffer.At<ValueType>(index.Value()))));
Indeed, that is an oversight.  Fixed.

http://codereview.chromium.org/8429027/diff/12001/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:128: if ((index.Value() < 0) || (index.Value() >=
buffer.Length())) {
Fixed.  This is all over array.cc, which is the origin of this method.  I'll
submit a cleanup for that file after this is submitted.

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/globals.h
File runtime/vm/globals.h (right):

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/globals.h#newcode36
runtime/vm/globals.h:36: #include <float.h>
{DBL,FLT}_{MIN,MAX}

I can move this in to the test code.  Or, I can leave this here and make
limits.h also a global include.

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/8429027/diff/12001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:727: RawObject** to() {
Sure.  Done.

[Ivan Posva, 2011-11-16 22:50:00]

http://codereview.chromium.org/8429027/diff/19002/runtime/lib/byte_buffer.cc
File runtime/lib/byte_buffer.cc (right):

http://codereview.chromium.org/8429027/diff/19002/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:131: if (index.Value() >= buffer.Length()) {
Don't you need to take the value size into account as well? What about < 0? I
find it confusing that it is different from the GetIndexed implementation, I
would have expected them to be similar.

http://codereview.chromium.org/8429027/diff/19002/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:153: SetIndexed<int8_t, Smi>(arguments);
Applies to other areas as well:
What if you have a Mint or BigInt that contains a small enough value?

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

http://codereview.chromium.org/8429027/diff/19002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6718: return !memcmp(this->Addr<uint8_t>(0),
other_array.Addr<uint8_t>(0), len);
Please do not use ints as boolean:
return memcmp(....) == 0;

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

http://codereview.chromium.org/8429027/diff/19002/runtime/vm/object.h#newcode...
runtime/vm/object.h:2806: ASSERT(Utils::IsAligned(byte_offset, sizeof(T)));
Shouldn't this check for (base_address + byte_offset) to be aligned properly?
Put differently, this should also work:

T* addr = Addr<T>(byte_offset);
ASSERT(Utils::IsAligned(addr, sizeof(T));
return *addr;

http://codereview.chromium.org/8429027/diff/19002/runtime/vm/object.h#newcode...
runtime/vm/object.h:2811: ASSERT(Utils::IsAligned(byte_offset, sizeof(T)));
ditto

[cshapiro, 2011-11-17 03:17:02]

http://codereview.chromium.org/8429027/diff/19002/runtime/lib/byte_buffer.cc
File runtime/lib/byte_buffer.cc (right):

http://codereview.chromium.org/8429027/diff/19002/runtime/lib/byte_buffer.cc#...
runtime/lib/byte_buffer.cc:153: SetIndexed<int8_t, Smi>(arguments);
Based on a conversation with Srdjan, a value in the Smi range will always be
represented as a Smi.  Same goes for Mint.  Am I confused?

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

http://codereview.chromium.org/8429027/diff/19002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:6718: return !memcmp(this->Addr<uint8_t>(0),
other_array.Addr<uint8_t>(0), len);
Certainly.  Done.

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

http://codereview.chromium.org/8429027/diff/19002/runtime/vm/object.h#newcode...
runtime/vm/object.h:2806: ASSERT(Utils::IsAligned(byte_offset, sizeof(T)));
It depends on whether the base address will ever be misaligned.  Either way, it
does not hurt to make the change you propose, so I will go ahead and
future-proof the code. Done.

http://codereview.chromium.org/8429027/diff/19002/runtime/vm/object.h#newcode...
runtime/vm/object.h:2811: ASSERT(Utils::IsAligned(byte_offset, sizeof(T)));
Done.