Merged String subclasses into String.

Merged String subclasses into String.

Merged String subclasses (OneByteString, etc.) into String, keeping
their raw types.  This allows String handles to continue to point
to a String after its raw type is changed, such as to make its data
external.


BUG=

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

[siva, 2012-11-02 01:15:25]

Initial comments.

I was unable to add any comments to object.cc
So I have tried to capture some of the comments I
tried to add inline here:


+  String& function_name =
+      OneByteString::Handle(isolate, String::null());
=====================================================
(Draft) 2012/11/01 23:38:20
Should be String::Handle(isolate, String::null());

There are no OneByteString handles.
Edit
======================================================
   intptr_t len = funcs.Length();
   for (intptr_t i = 0; i < len; i++) {
     function ^= funcs.At(i);
     function_name ^= function.name();
-    if (function_name.EqualsIgnoringPrivateKey(lookup_name)) {
+    if (OneByteString::EqualsIgnoringPrivateKey(function_name, name)) {
       return function.raw();


-      if (IsSpecialCharacter(*CharAddr(i))) {
-        *(dststr.CharAddr(index)) = '\\';
-        *(dststr.CharAddr(index + 1)) = SpecialCharacter(*CharAddr(i));
+      if (IsSpecialCharacter(str_chars[i])) {
=======================================================
(Draft) 2012/11/02 00:54:14
str_chars a raw pointer is live here across an allocation which could have
triggered a GC.

str_chars and dst_chars which are raw pointers is being
used here without any NoGCScope protection. Why not just
follow the old code model but as
*OneByteString::CharAddr(dststr, index)...
Edit
=======================================================
+        dst_chars[index] = '\\';
+        dst_chars[index + 1] = SpecialCharacter(str_chars[i]);
         index += 2;
-      } else if (!raw_str && (*CharAddr(i) == '\\')) {


+RawTwoByteString* TwoByteString::EscapeSpecialCharacters(const String& str,
+                                                         bool raw_str) {
+  intptr_t len = str.Length();
   if (len > 0) {
     intptr_t num_escapes = 0;
     intptr_t index = 0;
+    uint16_t* str_chars = CharAddr(str, 0);
     for (intptr_t i = 0; i < len; i++) {
-      if (IsSpecialCharacter(*CharAddr(i)) ||
-          (!raw_str && (*CharAddr(i) == '\\'))) {
+      if (IsSpecialCharacter(str_chars[i]) ||
=======================================================
(Draft) 2012/11/02 00:48:31
str_chars which is a raw pointer is being used in the loop and there is no
NOGCScope to protect it.

why not just use
If (IsSpecialCharacter(*TwoByteString::CharAddr(str, 0))...
Edit
=========================================================
+          (!raw_str && (str_chars[i] == '\\'))) {
         num_escapes += 1;
       }
     }
-    const TwoByteString& dststr = TwoByteString::Handle(
+    const String& dststr = String::Handle(
         TwoByteString::New(len + num_escapes, Heap::kNew));
+    uint16_t* dst_chars = CharAddr(dststr, 0);
     for (intptr_t i = 0; i < len; i++) {
-      if (IsSpecialCharacter(*CharAddr(i))) {
-        *(dststr.CharAddr(index)) = '\\';
-        *(dststr.CharAddr(index + 1)) = SpecialCharacter(*CharAddr(i));
+      if (IsSpecialCharacter(str_chars[i])) {
=========================================================
(Draft) 2012/11/02 00:51:26
str_chars a raw pointer is live here across an allocation that can trigger a GC.
Similarly dst_chars a raw pointer is used in the loop and there is no NOGcScope.
Edit
=========================================================
+        dst_chars[index] = '\\';
+        dst_chars[index + 1] = SpecialCharacter(str_chars[i]);
         index += 2;
-      } else if (!raw_str && (*CharAddr(i) == '\\')) {
-        *(dststr.CharAddr(index)) = '\\';
-        *(dststr.CharAddr(index + 1)) = '\\';

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

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3685
runtime/vm/object.h:3685: bool IsExternal() const;
why not inline this too:
{
  return IsExternalOneByteString() || IsExternalTwoByteString();
}

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3807
runtime/vm/object.h:3807: class OneByteString : public String {
We does this have to inherit from String

Shouldn't it be:
class OneByteString : public AllStatic {

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3871
runtime/vm/object.h:3871: Snapshot::Kind kind);
Why is this a public method for all other classes this is
a private method with a
friend class SnapshotReader;
declaration.

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3880
runtime/vm/object.h:3880: static uint8_t* CharAddr(const String& str, intptr_t
index) {
The index assertion seems to have been dropped..

Also need an assertion
ASSERT(str.IsOneByteString());

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3892
runtime/vm/object.h:3892: class TwoByteString : public String {
Ditto comment about this inheriting from String.
Should instead inherit from AllStatic

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3943
runtime/vm/object.h:3943: Snapshot::Kind kind);
Ditto comment about this being a public method.

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3952
runtime/vm/object.h:3952: static uint16_t* CharAddr(const String& str, intptr_t
index) {
The index assertion seems to have been dropped.

Also need assertion
ASSERT(str.IsTwoByteString());

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3964
runtime/vm/object.h:3964: class ExternalOneByteString : public String {
Ditto comment about inheriting from String, should instead
inherit from AllStatic

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode3996
runtime/vm/object.h:3996: Snapshot::Kind kind);
Ditto comment about this being a public method.

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode4006
runtime/vm/object.h:4006: ASSERT((index >= 0) && (index < str.Length()));
ASSERT(str.IsExternalOneByteString());

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode4012
runtime/vm/object.h:4012: ExternalStringData<uint8_t>* data) {
ASSERT(str.IsExternalOneByteString());

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode4024
runtime/vm/object.h:4024: class ExternalTwoByteString : public String {
Ditto comment about inheriting from AllStatic not String.

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode4056
runtime/vm/object.h:4056: Snapshot::Kind kind);
Ditto comment about being public.

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode4066
runtime/vm/object.h:4066: ASSERT((index >= 0) && (index < str.Length()));
ASSERT(str.IsExternalTwoByteString());

http://codereview.chromium.org/11367044/diff/1/runtime/vm/object.h#newcode4072
runtime/vm/object.h:4072: ExternalStringData<uint16_t>* data) {
ASSERT(str.IsExternalTwoByteString());
NoGCScope no_gc;

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

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object.h#newcode90
runtime/vm/raw_object.h:90: #define CLASS_LIST(V)                               
                          \
CLASS_LIST should include all classes right, now it does
not include all the string types.

Maybe it might be better if you had a
CLASS_LIST_NO_OBJECT_OR_STRING (contains all classes but strings)

CLASS_LIST_STRING (contains string and it's subtypes).

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

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object_snapshot...
runtime/vm/raw_object_snapshot.cc:1653: dst_chars[i] =
reader->Read<CharacterType>();
dst_chars is pointing to a raw pointer and we are looping through without any
NOGCScope here.

why not leave the loop as is, that should work right?

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object_snapshot...
runtime/vm/raw_object_snapshot.cc:1667: String& str_obj =
OneByteString::ZoneHandle(reader->isolate(),
Should be String::ZoneHandle(.....);

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object_snapshot...
runtime/vm/raw_object_snapshot.cc:1678:
const_cast<uint8_t*>(OneByteString::CharAddr(str_obj, 0));
why is this const_cast needed?

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object_snapshot...
runtime/vm/raw_object_snapshot.cc:1686: return
reinterpret_cast<RawOneByteString*>(str_obj.raw());
I am not sure how many places you are having to do this
kind of cast , it might make sense to have a
OneByteString::raw(str_obj);
which does the case inside and returns.

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object_snapshot...
runtime/vm/raw_object_snapshot.cc:1698: String& str_obj =
TwoByteString::ZoneHandle(reader->isolate(),
Should be String::ZoneHandle(...);

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object_snapshot...
runtime/vm/raw_object_snapshot.cc:1707:
const_cast<uint16_t*>(TwoByteString::CharAddr(str_obj, 0)) : NULL;
Why is this const_cast needed?

http://codereview.chromium.org/11367044/diff/1/runtime/vm/raw_object_snapshot...
runtime/vm/raw_object_snapshot.cc:1719: return
reinterpret_cast<RawTwoByteString*>(str_obj.raw());
Ditto comment for TwoByteString::raw(str_obj);

[Tom Ball, 2012-11-02 23:31:50]

All done, I think.  The string raw classes extend AllStatic, CharAddr use is
restored to the original style, and raw() methods now hide the reinterpret_cast
noise.

With the new delta of object.cc, Rietveld now shows its diff view.  I took the
liberty of cut&pasting your initial review comments into that file, then
responded to each of them in-line.

Question:  with a static method like raw(), do you prefer
"return raw(result);" or "return OneByteString::raw(result);"?  The first is
simpler, but the second makes it more obvious what's being returned.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
File runtime/vm/object.cc (right):

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.cc:2201: OneByteString::Handle(isolate, String::null());
> Should be String::Handle(isolate, String::null());
> 
> There are no OneByteString handles.

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.cc:10323: (!raw_str && (str_chars[i] == '\\'))) {
> str_chars a raw pointer is live here across an allocation which could have
> triggered a GC.
> 
> str_chars and dst_chars which are raw pointers is being
> used here without any NoGCScope protection. Why not just
> follow the old code model but as
> *OneByteString::CharAddr(dststr, index)...

The code was getting uglier and harder to understand, and I thought a 
single pointer increment would be faster than a function call/two-pointer-
dereferences/pointer-subtraction, especially for string copies 
where that gets done for each character of both the source and dest
strings.  I put it back, and will investigate whether there's actually a
performance gain in a separate CL, as you suggested.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.cc:10532: (!raw_str && (str_chars[i] == '\\'))) {
> str_chars which is a raw pointer is being used in the loop and there is no
> NOGCScope to protect it.
> 
> why not just use
> If (IsSpecialCharacter(*TwoByteString::CharAddr(str, 0))...

Restored.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.cc:10540: if (IsSpecialCharacter(str_chars[i])) {
> str_chars a raw pointer is live here across an allocation that can trigger a
GC.
> Similarly dst_chars a raw pointer is used in the loop and there is no
NOGcScope.

Restored.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.cc:10540: if (IsSpecialCharacter(str_chars[i])) {
> str_chars a raw pointer is live here across an allocation that can trigger a
GC.
> Similarly dst_chars a raw pointer is used in the loop and there is no
NOGcScope.

Restored.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
File runtime/vm/object.h (right):

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3685: bool IsExternal() const;
On 2012/11/02 01:15:25, siva wrote:
> why not inline this too:
> {
>   return IsExternalOneByteString() || IsExternalTwoByteString();
> }

Good idea -- I also found that RawObject::IsExternalStringClassId() has the same
assertion, so used that inlined function.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3807: class OneByteString : public String {
On 2012/11/02 01:15:25, siva wrote:
> We does this have to inherit from String
> 
> Shouldn't it be:
> class OneByteString : public AllStatic {

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3871: Snapshot::Kind kind);
On 2012/11/02 01:15:25, siva wrote:
> Why is this a public method for all other classes this is
> a private method with a
> friend class SnapshotReader;
> declaration.

Made private for all four string classes.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3880: static uint8_t* CharAddr(const String& str, intptr_t
index) {
On 2012/11/02 01:15:25, siva wrote:
> The index assertion seems to have been dropped..
> 
> Also need an assertion
> ASSERT(str.IsOneByteString());

Restored old assertion and added new one.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3892: class TwoByteString : public String {
On 2012/11/02 01:15:25, siva wrote:
> Ditto comment about this inheriting from String.
> Should instead inherit from AllStatic

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3943: Snapshot::Kind kind);
On 2012/11/02 01:15:25, siva wrote:
> Ditto comment about this being a public method.

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3952: static uint16_t* CharAddr(const String& str, intptr_t
index) {
On 2012/11/02 01:15:25, siva wrote:
> The index assertion seems to have been dropped.
> 
> Also need assertion
> ASSERT(str.IsTwoByteString());

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3964: class ExternalOneByteString : public String {
On 2012/11/02 01:15:25, siva wrote:
> Ditto comment about inheriting from String, should instead
> inherit from AllStatic

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:3996: Snapshot::Kind kind);
On 2012/11/02 01:15:25, siva wrote:
> Ditto comment about this being a public method.

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:4006: ASSERT((index >= 0) && (index < str.Length()));
On 2012/11/02 01:15:25, siva wrote:
> ASSERT(str.IsExternalOneByteString());

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:4012: ExternalStringData<uint8_t>* data) {
On 2012/11/02 01:15:25, siva wrote:
> ASSERT(str.IsExternalOneByteString());

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:4024: class ExternalTwoByteString : public String {
On 2012/11/02 01:15:25, siva wrote:
> Ditto comment about inheriting from AllStatic not String.

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:4056: Snapshot::Kind kind);
On 2012/11/02 01:15:25, siva wrote:
> Ditto comment about being public.

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:4066: ASSERT((index >= 0) && (index < str.Length()));
On 2012/11/02 01:15:25, siva wrote:
> ASSERT(str.IsExternalTwoByteString());

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/object.h:4072: ExternalStringData<uint16_t>* data) {
On 2012/11/02 01:15:25, siva wrote:
> ASSERT(str.IsExternalTwoByteString());
> NoGCScope no_gc;

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
File runtime/vm/raw_object.h (right):

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object.h:90: #define CLASS_LIST(V)                               
                          \
On 2012/11/02 01:15:25, siva wrote:
> CLASS_LIST should include all classes right, now it does
> not include all the string types.
> 
> Maybe it might be better if you had a
> CLASS_LIST_NO_OBJECT_OR_STRING (contains all classes but strings)
> 
> CLASS_LIST_STRING (contains string and it's subtypes).

Fixed CLASS_LIST and CLASS_LIST_NO_OBJECT to include all string classes, 
CLASS_LIST_NO_OBJECT_OR_STRINGS (no strings at all), CLASS_LISTS_STRINGS (just
strings), and CLASS_LIST_NO_OBJECT_WITH_STRING (just String, no String raw
types).

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
File runtime/vm/raw_object_snapshot.cc (right):

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object_snapshot.cc:1653: dst_chars[i] =
reader->Read<CharacterType>();
On 2012/11/02 01:15:25, siva wrote:
> dst_chars is pointing to a raw pointer and we are looping through without any
> NOGCScope here.
> 
> why not leave the loop as is, that should work right?  

Restored original loop, updated it.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object_snapshot.cc:1667: String& str_obj =
OneByteString::ZoneHandle(reader->isolate(),
On 2012/11/02 01:15:25, siva wrote:
> Should be String::ZoneHandle(.....);

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object_snapshot.cc:1678:
const_cast<uint8_t*>(OneByteString::CharAddr(str_obj, 0));
On 2012/11/02 01:15:25, siva wrote:
> why is this const_cast needed?

Removed.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object_snapshot.cc:1686: return
reinterpret_cast<RawOneByteString*>(str_obj.raw());
On 2012/11/02 01:15:25, siva wrote:
> I am not sure how many places you are having to do this
> kind of cast , it might make sense to have a
> OneByteString::raw(str_obj);
> which does the case inside and returns.

Good idea -- done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object_snapshot.cc:1698: String& str_obj =
TwoByteString::ZoneHandle(reader->isolate(),
On 2012/11/02 01:15:25, siva wrote:
> Should be String::ZoneHandle(...);

Done.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object_snapshot.cc:1707:
const_cast<uint16_t*>(TwoByteString::CharAddr(str_obj, 0)) : NULL;
On 2012/11/02 01:15:25, siva wrote:
> Why is this const_cast needed?

Removed.

http://r925-3ff970138a33.chromiumcodereview-hr.appspot.com/11367044/diff/1/ru...
runtime/vm/raw_object_snapshot.cc:1719: return
reinterpret_cast<RawTwoByteString*>(str_obj.raw());
On 2012/11/02 01:15:25, siva wrote:
> Ditto comment for TwoByteString::raw(str_obj);

Done.

[siva, 2012-11-05 19:23:38]

Some minor comments.

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:145: CLASS_LIST_NO_OBJECT_WITH_STRING(DEFINE_UNWRAP)
Ditto comment about readability of this.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/dart_api_impl.h
File runtime/vm/dart_api_impl.h (right):

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/dart_api_impl.h#...
runtime/vm/dart_api_impl.h:84: CLASS_LIST_NO_OBJECT_WITH_STRING(DECLARE_UNWRAP)
CLASS_LIST_NO_OBJECT_WITH_STRING is kind of hard to read
(does it mean no object_with_string is included in the list)?

Maybe we could spell it out explicitly here:
#define DECLARE_UNWRAP(type)
.....
CLASS_LIST_NO_OBJECT_OR_STRING(DECLARE_UNWRAP)
DECLARE_UNWRAP(String)
#undef DECLARE_UNWRAP

or have a macro
#define CLASS_LIST_FOR_HANDLES(V)                          \
  CLASS_LIST_NO_OBJECT_OR_STRING                           \
  V(String)
defined in object.h

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:1882: Class& result = Class::Handle(New<String>(class_id));
I would pull this down to after the instance_size is set.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10388: }
Should this function still work if str2 is a TwoByteString?

The old code only accepted one byte strings.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10440: RawObject* raw = Object::Allocate(kClassId,
OneByteString::kClassId
(reads better and would be consistent with InstanceSize etc.)

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10585: RawObject* raw = Object::Allocate(kClassId,
TwoByteString::kClassId

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10720: RawObject* raw = Object::Allocate(kClassId,
ExternalOneByteString::kClassId

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10767: RawObject* raw = Object::Allocate(kClassId,
ExternalTwoByteString::kClassId

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.h#newcode...
runtime/vm/object.h:3811: template<typename HandleType, typename ElementType>
HandleType should probably be called StringType as it is not a handle type
anymore.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.h#newcode...
runtime/vm/object.h:4011: }
Normally we put private methods after all the public methods. This one is
dangling over here.

In the previous classes you have used raw_ptr get at the raw pointer but here
you have called raw_str why the distinction?

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.h#newcode...
runtime/vm/object.h:4078: }
Ditto comment here regarding private method and the name raw_str Vs raw_ptr

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1528: return (index >= kOneByteStringCid && index <=
kExternalTwoByteStringCid);
>= kStringCid (at least that is what the old code had).

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/raw_object_snaps...
File runtime/vm/raw_object_snapshot.cc (right):

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:1629: template<typename HandleType, typename
CharacterType>
HandleType should probably be renamed to StringType as it
is not a handle type anymore.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/snapshot.cc#newc...
runtime/vm/snapshot.cc:436: RawObject::IsStringClassId(class_id));
would this be equivalent to
(class_id >- kInstanceCid && class_id <= kExternalTwoByteStringCid)

[cshapiro, 2012-11-05 19:40:13]

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:9803: intptr_t class_id = raw()->GetClassId();
DBC - there are 3 different styles of doing type discrimination (switch, if and
assert after, if and assert in the else case).  Why not pick one and use it all
the time?

[Tom Ball, 2012-11-05 21:22:41]

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:145: CLASS_LIST_NO_OBJECT_WITH_STRING(DEFINE_UNWRAP)
On 2012/11/05 19:23:38, siva wrote:
> Ditto comment about readability of this.

Done.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/dart_api_impl.h
File runtime/vm/dart_api_impl.h (right):

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/dart_api_impl.h#...
runtime/vm/dart_api_impl.h:84: CLASS_LIST_NO_OBJECT_WITH_STRING(DECLARE_UNWRAP)
On 2012/11/05 19:23:38, siva wrote:
> CLASS_LIST_NO_OBJECT_WITH_STRING is kind of hard to read
> (does it mean no object_with_string is included in the list)?
> 
> Maybe we could spell it out explicitly here:
> #define DECLARE_UNWRAP(type)
> .....
> CLASS_LIST_NO_OBJECT_OR_STRING(DECLARE_UNWRAP)
> DECLARE_UNWRAP(String)
> #undef DECLARE_UNWRAP
> 
> or have a macro
> #define CLASS_LIST_FOR_HANDLES(V)                          \
>   CLASS_LIST_NO_OBJECT_OR_STRING                           \
>   V(String)
> defined in object.h

Implemented your macro suggestion.

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:1882: Class& result = Class::Handle(New<String>(class_id));
On 2012/11/05 19:23:38, siva wrote:
> I would pull this down to after the instance_size is set.

Done.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:9803: intptr_t class_id = raw()->GetClassId();
On 2012/11/05 19:40:13, cshapiro wrote:
> DBC - there are 3 different styles of doing type discrimination (switch, if
and
> assert after, if and assert in the else case).  Why not pick one and use it
all
> the time?

Updated to just use if statements.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10388: }
On 2012/11/05 19:23:38, siva wrote:
> Should this function still work if str2 is a TwoByteString?
> 
> The old code only accepted one byte strings.

This function is called by two functions that first check that it's a one-byte
string.  I added an assert statement to double-check that.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10440: RawObject* raw = Object::Allocate(kClassId,
On 2012/11/05 19:23:38, siva wrote:
> OneByteString::kClassId
> (reads better and would be consistent with InstanceSize etc.)

Done.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10585: RawObject* raw = Object::Allocate(kClassId,
On 2012/11/05 19:23:38, siva wrote:
> TwoByteString::kClassId

Done.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10720: RawObject* raw = Object::Allocate(kClassId,
On 2012/11/05 19:23:38, siva wrote:
> ExternalOneByteString::kClassId

Done.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:10767: RawObject* raw = Object::Allocate(kClassId,
On 2012/11/05 19:23:38, siva wrote:
> ExternalTwoByteString::kClassId

Done.

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.h#newcode...
runtime/vm/object.h:4011: }
On 2012/11/05 19:23:38, siva wrote:
> Normally we put private methods after all the public methods. This one is
> dangling over here.
> 
> In the previous classes you have used raw_ptr get at the raw pointer but here
> you have called raw_str why the distinction?

This method was added before the other methods were extracted.  I've renamed
them to raw_ptr and moved them down to the private section.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/object.h#newcode...
runtime/vm/object.h:4078: }
On 2012/11/05 19:23:38, siva wrote:
> Ditto comment here regarding private method and the name raw_str Vs raw_ptr

Done.

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

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/raw_object.h#new...
runtime/vm/raw_object.h:1528: return (index >= kOneByteStringCid && index <=
kExternalTwoByteStringCid);
On 2012/11/05 19:23:38, siva wrote:
> >= kStringCid (at least that is what the old code had).

Done.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/raw_object_snaps...
File runtime/vm/raw_object_snapshot.cc (right):

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/raw_object_snaps...
runtime/vm/raw_object_snapshot.cc:1629: template<typename HandleType, typename
CharacterType>
On 2012/11/05 19:23:38, siva wrote:
> HandleType should probably be renamed to StringType as it
> is not a handle type anymore.

Done.

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

http://codereview.chromium.org/11367044/diff/7001/runtime/vm/snapshot.cc#newc...
runtime/vm/snapshot.cc:436: RawObject::IsStringClassId(class_id));
On 2012/11/05 19:23:38, siva wrote:
> would this be equivalent to
> (class_id >- kInstanceCid && class_id <= kExternalTwoByteStringCid)

Done.

[siva, 2012-11-05 22:00:13]

lgtm