Added Object._cid getter, optimized it. Added to (some) classes a static final _clCid. Use those to…

Added Object._cid getter, optimized it. Added to (some) classes a static final _clCid. Use those to test for exact type in VM's libraries.

R=iposva@google.com

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

[srdjan, 2013-05-10 23:50:01]

This CL is something that was discussed before and I wonder if that is the
direction that we should be going to.

[siva, 2013-05-13 20:38:00]

https://codereview.chromium.org/14703005/diff/11/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/array.dart#newcode8
runtime/lib/array.dart:8: static final int _clCid = (new List(0))._cid;
clCid seems redundant 'class class id'
maybe _classId

https://codereview.chromium.org/14703005/diff/11/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/object.cc#newcode19
runtime/lib/object.cc:19: const Instance& instance =
Instance::CheckedHandle(arguments->NativeArgAt(0));
GET_NONE_NULL_NATIVE_ARGUMENT(Instance, instance, arguments->NativeArgAt(0));

If the object is NULL I don't think we will get the correct cid.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:35: (ccid != _ImmutableArray._clCid)) {
It might be more readable to have static methods in the Object class for these

Object.IsArray(charCodes);

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:346: if (isOneByteString && (s._cid ==
_OneByteString._clCid)) {
Object.IsOneByteString(s);

[Ivan Posva, 2013-05-13 22:12:37]

LGTMwC

As discussed we need a better way of creating these cids from the VMs C++ code,
but for now this should be fine.

-Ivan

https://codereview.chromium.org/14703005/diff/11/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/array.dart#newcode8
runtime/lib/array.dart:8: static final int _clCid = (new List(0))._cid;
On 2013/05/13 20:38:00, siva wrote:
> clCid seems redundant 'class class id'
> maybe _classId

I really like the suggestion of _classId. Also please allocate a _ObjectArray
here instead of relying on the List constructor doing the right thing.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/growable_array.dart
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/growable_array.d...
runtime/lib/growable_array.dart:6: static final int _clCid = []._cid;
Same problem about relying on the default List construction.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/growable_array.d...
runtime/lib/growable_array.dart:8: factory
_GrowableObjectArray._uninstantiable() {
Of topic: Is this constructor still needed here?

https://codereview.chromium.org/14703005/diff/11/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/object.cc#newcode19
runtime/lib/object.cc:19: const Instance& instance =
Instance::CheckedHandle(arguments->NativeArgAt(0));
On 2013/05/13 20:38:00, siva wrote:
> GET_NONE_NULL_NATIVE_ARGUMENT(Instance, instance, arguments->NativeArgAt(0));
> 
> If the object is NULL I don't think we will get the correct cid.

Null is a valid receiver for the _cid method. So I do think this code here is
correct.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:633: class _ExternalFourByteString extends
_StringBase implements String {
I don't think this needs to be here anymore. Four-byte strings are a thing of
the past.

[srdjan, 2013-05-14 17:46:31]

Thanks!

https://codereview.chromium.org/14703005/diff/11/runtime/lib/array.dart
File runtime/lib/array.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/array.dart#newcode8
runtime/lib/array.dart:8: static final int _clCid = (new List(0))._cid;
On 2013/05/13 20:38:00, siva wrote:
> clCid seems redundant 'class class id'
> maybe _classId

Done.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/array.dart#newcode8
runtime/lib/array.dart:8: static final int _clCid = (new List(0))._cid;
On 2013/05/13 22:12:37, Ivan Posva wrote:
> On 2013/05/13 20:38:00, siva wrote:
> > clCid seems redundant 'class class id'
> > maybe _classId
> 
> I really like the suggestion of _classId. Also please allocate a _ObjectArray
> here instead of relying on the List constructor doing the right thing.

Done.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/growable_array.dart
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/growable_array.d...
runtime/lib/growable_array.dart:6: static final int _clCid = []._cid;
On 2013/05/13 22:12:37, Ivan Posva wrote:
> Same problem about relying on the default List construction.

Done.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/growable_array.d...
runtime/lib/growable_array.dart:8: factory
_GrowableObjectArray._uninstantiable() {
On 2013/05/13 22:12:37, Ivan Posva wrote:
> Of topic: Is this constructor still needed here?

Removed

https://codereview.chromium.org/14703005/diff/11/runtime/lib/object.cc
File runtime/lib/object.cc (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/object.cc#newcode19
runtime/lib/object.cc:19: const Instance& instance =
Instance::CheckedHandle(arguments->NativeArgAt(0));
On 2013/05/13 22:12:37, Ivan Posva wrote:
> On 2013/05/13 20:38:00, siva wrote:
> > GET_NONE_NULL_NATIVE_ARGUMENT(Instance, instance,
arguments->NativeArgAt(0));
> > 
> > If the object is NULL I don't think we will get the correct cid.
> 
> Null is a valid receiver for the _cid method. So I do think this code here is
> correct.

What Ivan said.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:35: (ccid != _ImmutableArray._clCid)) {
On 2013/05/13 20:38:00, siva wrote:
> It might be more readable to have static methods in the Object class for these
> 
> Object.IsArray(charCodes);

I am open to change it, but I think this is more readable. I would be more in
favor of the change if we need the same test on various places. TODO says that
we need to add typed array as well. IsArray should return true for typed array
as well, but createFromCharCodes does not support it yet.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:346: if (isOneByteString && (s._cid ==
_OneByteString._clCid)) {
On 2013/05/13 20:38:00, siva wrote:
> Object.IsOneByteString(s);

Next CL as discussed offline.

https://codereview.chromium.org/14703005/diff/11/runtime/lib/string_patch.dar...
runtime/lib/string_patch.dart:633: class _ExternalFourByteString extends
_StringBase implements String {
On 2013/05/13 22:12:37, Ivan Posva wrote:
> I don't think this needs to be here anymore. Four-byte strings are a thing of
> the past.

Removed.

[srdjan, 2013-05-14 18:16:51]

Committed patchset #5 manually as r22695 (presubmit successful).