Setup a peer for read only strings that are not externalized in

runtime/vm/dart_api_impl.cc

Index: runtime/vm/dart_api_impl.cc
===================================================================
--- runtime/vm/dart_api_impl.cc	(revision 26385)
+++ runtime/vm/dart_api_impl.cc	(working copy)
@@ -240,47 +240,34 @@
 }
 
 
-#define GET_PEER(type, ctype, raw_obj, peer)                                   \
+#define EXTERNAL_PEER_HELPER(type, ctype, raw_obj, peer)                       \

[Ivan Posva, 2013/08/22 15:44:51]

Is this #define really worth the added confusion? To me it feels like the code
would be more readable if we just wrote the code twice with the appropriate
types inline.

[siva, 2013/08/22 16:57:08]

Removed the macro and inline the code below.

On 2013/08/22 15:44:51, Ivan Posva wrote:

+  type* raw_string = reinterpret_cast<type*>(raw_obj)->ptr();                  \
+  ExternalStringData<ctype>* data = raw_string->external_data_;                \
+  *peer = data->peer();                                                        \
 
-
-#define EXTERNAL_PEER_HELPER(cid, raw_obj, peer)                               \
-  switch (cid) {                                                               \
-    case kExternalOneByteStringCid: {                                          \
-      RawExternalOneByteString* raw_string =                                   \
-          reinterpret_cast<RawExternalOneByteString*>(raw_obj)->ptr();         \
-      ExternalStringData<uint8_t>* data = raw_string->external_data_;          \
-      *peer = data->peer();                                                    \
-      return true;                                                             \
-    }                                                                          \
-    case kExternalTwoByteStringCid: {                                          \
-      RawExternalTwoByteString* raw_string =                                   \
-          reinterpret_cast<RawExternalTwoByteString*>(raw_obj)->ptr();         \
-      ExternalStringData<uint16_t>* data = raw_string->external_data_;         \
-      *peer = data->peer();                                                    \
-      return true;                                                             \
-    }                                                                          \
-  }                                                                            \
-  return false;                                                                \
-
-
-bool Api::ExternalStringGetPeerHelper(Dart_NativeArguments args,
-                                      int arg_index,
-                                      void** peer) {
+bool Api::StringGetPeerHelper(Dart_NativeArguments args,
+                              int arg_index,
+                              void** peer) {
   NoGCScope no_gc_scope;
   NativeArguments* arguments = reinterpret_cast<NativeArguments*>(args);
   RawObject* raw_obj = arguments->NativeArgAt(arg_index);
   intptr_t cid = raw_obj->GetClassId();
-  EXTERNAL_PEER_HELPER(cid, raw_obj, peer);
+  if (cid == kExternalOneByteStringCid) {
+    EXTERNAL_PEER_HELPER(RawExternalOneByteString, uint8_t, raw_obj, peer);
+    return true;
+  }
+  if (cid == kOneByteStringCid || cid == kTwoByteStringCid) {
+    *peer = Isolate::Current()->heap()->GetPeer(raw_obj);

[Ivan Posva, 2013/08/22 15:44:51]

args carries the isolate or pass it in from the caller?

[siva, 2013/08/22 16:57:08]

Good point.

On 2013/08/22 15:44:51, Ivan Posva wrote:

+    return (*peer != 0);
+  }
+  if (cid == kExternalTwoByteStringCid) {
+    EXTERNAL_PEER_HELPER(RawExternalTwoByteString, uint16_t, raw_obj, peer);
+    return true;
+  }
+  return false;
 }
 
 
-bool Api::ExternalStringGetPeerHelper(Dart_Handle object, void** peer) {
-  NoGCScope no_gc_scope;
-  RawObject* raw_obj = Api::UnwrapHandle(object);
-  EXTERNAL_PEER_HELPER(Api::ClassId(object), raw_obj, peer);
-}
-
-
 void Api::SetWeakHandleReturnValue(NativeArguments* args,
                                    Dart_WeakPersistentHandle retval) {
   args->SetReturnUnsafe(Api::UnwrapAsWeakPersistentHandle(retval)->raw());
@@ -1647,28 +1634,6 @@
 }
 
 
-DART_EXPORT Dart_Handle Dart_ExternalStringGetPeer(Dart_Handle object,
-                                                   void** peer) {
-  if (peer == NULL) {
-    RETURN_NULL_ERROR(peer);
-  }
-
-  if (Api::ExternalStringGetPeerHelper(object, peer)) {
-    return Api::Success();
-  }
-
-  // It's not an external string, return appropriate error.
-  if (!RawObject::IsStringClassId(Api::ClassId(object))) {
-    RETURN_TYPE_ERROR(Isolate::Current(), object, String);
-  } else {
-    return
-        Api::NewError(
-            "%s expects argument 'object' to be an external String.",
-            CURRENT_FUNC);
-  }
-}
-
-
 DART_EXPORT Dart_Handle Dart_NewExternalLatin1String(
     const uint8_t* latin1_array,
     intptr_t length,
@@ -1842,9 +1807,9 @@
   if (str_obj.InVMHeap()) {
     // Since the string object is read only we do not externalize
     // the string but instead copy the contents of the string into the
-    // specified buffer and return a Null object.
-    // This ensures that the embedder does not have to call again
-    // to get at the contents.
+    // specified buffer add the specified peer/cback as a Peer object
+    // to this string. The Api::StringGetPeerHelper function picks up
+    // the peer from the Peer table.
     intptr_t copy_len = str_obj.Length();
     if (str_obj.IsOneByteString()) {
       ASSERT(length >= copy_len);
@@ -1852,6 +1817,7 @@
       for (intptr_t i = 0; i < copy_len; i++) {
         latin1_array[i] = static_cast<uint8_t>(str_obj.CharAt(i));
       }
+      OneByteString::SetPeer(str_obj, peer, cback);
     } else {
       ASSERT(str_obj.IsTwoByteString());
       ASSERT(length >= (copy_len * str_obj.CharSize()));
@@ -1859,8 +1825,9 @@
       for (intptr_t i = 0; i < copy_len; i++) {
         utf16_array[i] = static_cast<uint16_t>(str_obj.CharAt(i));
       }
+      TwoByteString::SetPeer(str_obj, peer, cback);
     }
-    return Api::Null();
+    return str;
   }
   return Api::NewHandle(isolate,
                         str_obj.MakeExternal(array, length, peer, cback));
@@ -1881,7 +1848,8 @@
     *peer = str.GetPeer();
     ASSERT(*peer != NULL);
   } else {
-    *peer = NULL;
+    NoGCScope no_gc_scope;
+    *peer = isolate->heap()->GetPeer(str.raw());
   }
   *char_size = str.CharSize();
   *str_len = str.Length();
@@ -3704,7 +3672,7 @@
   NativeArguments* arguments = reinterpret_cast<NativeArguments*>(args);
   Isolate* isolate = arguments->isolate();
   CHECK_ISOLATE(isolate);
-  if (Api::ExternalStringGetPeerHelper(args, arg_index, peer)) {
+  if (Api::StringGetPeerHelper(args, arg_index, peer)) {
     return Api::Success();

[Ivan Posva, 2013/08/22 15:44:51]

This function does not do what it is documented to do: Return the string object
handle if no error occurs.

[siva, 2013/08/22 16:57:08]

Updated comment to reflect this. If a peer is present then the
caller is only interested in the peer.

On 2013/08/22 15:44:51, Ivan Posva wrote:

   }
   *peer = NULL;