Reified metadata in the VM

Reified metadata in the VM

R=asiva@google.com

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

[siva, 2013-06-12 00:16:52]

LGTM

https://codereview.chromium.org/16780008/diff/6002/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/include/dart_api.h...
runtime/include/dart_api.h:2286: *   Returns and empty array if there is no
metadata for the object.
Returns an

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4366: return Api::NewHandle(isolate, Array::New(0));
return Api::NewHandle(isolate, Object::empty_array().raw());

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4370: return Api::NewHandle(isolate, obj.raw());
why not:

return Api::NewHandle(isolate, lib.GetMetadata(obj));

would avoid an unnecessary call to SetRaw().

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6005: return String::Concat(cname, fname);
Maybe more efficient to use ConcatAll
String name = String::Handle();
const Array& names = Array::Handle(Array::New(4));

names.SetAt(0, Symbols::At());
name = cls.Name();
names.SetAt(1, name);
names.SetAt(2, Symbols::At());
name = field.Name();
names.SetAt(3, name);
return String::ConcatAll(names);

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6014: return String::Concat(cname, fname);
Ditto here.

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6032: metadata.Add(meta_field, Heap::kOld);
Seems like the above code could be abstracted into a static helper method and
reused in AddFieldMetadata and AddFunctionMetadata

static void AddMetaData(const String& metaname,
                        const Class& cls,
                        intptr_t token_pos) {
  .... (code starting at line 6020).
}

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6117: if (metadata.IsArray()) {
ASSERT(Array::Cast(metadata).raw() != Object::empty_array().raw());

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6663: result.raw_ptr()->metadata_ =
GrowableObjectArray::New(4, Heap::kOld);
result.StorePointer(&result.raw_ptr()->metadata_, GrowableObjectArray::New(4,
Heap::kOld));

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object_test.cc#...
runtime/vm/object_test.cc:3250: PrintMetadata("A", res);
Instead of PrintMetadata why not convert this to
ExpectedMetadata so that it self verifies.

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:898: }
ASSERT(meta_values.Length() > 0); ?

We don't expect to end up with empty arrays here I suppose.

[hausner, 2013-06-12 01:10:04]

Thank you.

https://codereview.chromium.org/16780008/diff/6002/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/include/dart_api.h...
runtime/include/dart_api.h:2286: *   Returns and empty array if there is no
metadata for the object.
On 2013/06/12 00:16:52, siva wrote:
> Returns an

Done.

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4366: return Api::NewHandle(isolate, Array::New(0));
On 2013/06/12 00:16:52, siva wrote:
> return Api::NewHandle(isolate, Object::empty_array().raw());

Done.

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:4370: return Api::NewHandle(isolate, obj.raw());
On 2013/06/12 00:16:52, siva wrote:
> why not:
> 
> return Api::NewHandle(isolate, lib.GetMetadata(obj));
> 
> would avoid an unnecessary call to SetRaw().

Done.

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6005: return String::Concat(cname, fname);
On 2013/06/12 00:16:52, siva wrote:
> Maybe more efficient to use ConcatAll
> String name = String::Handle();
> const Array& names = Array::Handle(Array::New(4));
> 
> names.SetAt(0, Symbols::At());
> name = cls.Name();
> names.SetAt(1, name);
> names.SetAt(2, Symbols::At());
> name = field.Name();
> names.SetAt(3, name);
> return String::ConcatAll(names);

I am not terribly concerned about speed here. ConcatAll seems less readable to
me.

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6032: metadata.Add(meta_field, Heap::kOld);
On 2013/06/12 00:16:52, siva wrote:
> Seems like the above code could be abstracted into a static helper method and
> reused in AddFieldMetadata and AddFunctionMetadata
> 
> static void AddMetaData(const String& metaname,
>                         const Class& cls,
>                         intptr_t token_pos) {
>   .... (code starting at line 6020).
> }

Done.

https://codereview.chromium.org/16780008/diff/6002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6117: if (metadata.IsArray()) {
On 2013/06/12 00:16:52, siva wrote:
> ASSERT(Array::Cast(metadata).raw() != Object::empty_array().raw());

Done.

[hausner, 2013-06-12 15:45:03]

Committed patchset #3 manually as r23912 (presubmit successful).