[builtins] move String.prototype[@@iterator] to C++ builtin

src/bootstrapper.cc

 // Copyright 2014 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/bootstrapper.h"
 
 #include "src/accessors.h"
 #include "src/api-natives.h"
 #include "src/base/ieee754.h"
 #include "src/code-stubs.h"
@@ skipping 1388 matching lines @@
     SimpleInstallFunction(prototype, "toString",
                           Builtins::kStringPrototypeToString, 0, true);
     SimpleInstallFunction(prototype, "trim", Builtins::kStringPrototypeTrim, 0,
                           false);
     SimpleInstallFunction(prototype, "trimLeft",
                           Builtins::kStringPrototypeTrimLeft, 0, false);
     SimpleInstallFunction(prototype, "trimRight",
                           Builtins::kStringPrototypeTrimRight, 0, false);
     SimpleInstallFunction(prototype, "valueOf",
                           Builtins::kStringPrototypeValueOf, 0, true);
+
+    Handle<JSFunction> iterator = CreateFunction(
+        isolate, factory->NewStringFromAsciiChecked("[Symbol.iterator]"),
+        JS_OBJECT_TYPE, JSObject::kHeaderSize, MaybeHandle<JSObject>(),
+        Builtins::kStringCreateIterator, true);
+    iterator->shared()->set_internal_formal_parameter_count(0);
+    iterator->shared()->set_length(0);
+    iterator->shared()->set_language_mode(STRICT);

[Benedikt Meurer, 2016/09/15 17:34:33]

Why do you need to set the language mode on a C++ builtin?

[caitp, 2016/09/15 19:02:39]

I guess the other builtins just use set_native(true), so I've done that. A
better fix would be to add a version of these helpers which accept symbol
properties, but that seemed like more effort

[Benedikt Meurer, 2016/09/16 03:16:37]

Yes, we really need a version that can deal with names in general, but Yang
and/or Andreas expressed some concerns regarding the number of helpers...

+    iterator->map()->set_is_constructor(false);

[Benedikt Meurer, 2016/09/15 17:34:33]

This mutates the map inplace, which is not what you want. You have to use the
appropriate map instead.

+
+    JSObject::AddProperty(prototype, factory->iterator_symbol(), iterator,
+                          static_cast<PropertyAttributes>(DONT_ENUM));
   }
 
   {
     // --- S y m b o l ---
     Handle<JSObject> prototype =
         factory->NewJSObject(isolate->object_function(), TENURED);
     Handle<JSFunction> symbol_fun =
         InstallFunction(global, "Symbol", JS_VALUE_TYPE, JSValue::kSize,
                         prototype, Builtins::kSymbolConstructor);
     symbol_fun->shared()->SetConstructStub(
@@ skipping 1956 matching lines @@
     }
     {
       AccessorConstantDescriptor d(factory()->iterator_symbol(),
                                    arguments_iterator, attribs);
       Handle<Map> map(native_context()->strict_arguments_map());
       Map::EnsureDescriptorSlack(map, 1);
       map->AppendDescriptor(&d);
     }
   }
 
+  {  // StringIterator
+    PrototypeIterator iter(native_context()->generator_object_prototype_map());

[Benedikt Meurer, 2016/09/15 17:34:33]

Can we make this more robust? I.e. have the iterator prototype as field in the
native context or the bootstrapper?

[caitp, 2016/09/15 19:02:39]

Ok.

I assume it wasn't done originally to avoid growing the context more.

+    iter.Advance();  // Advance to the prototype of generator_object_prototype.
+    Handle<JSObject> iterator_prototype =
+        Handle<JSObject>(iter.GetCurrent<JSObject>());
+
+    Handle<JSObject> string_iterator_prototype =
+        factory()->NewJSObject(isolate()->object_function(), TENURED);
+    JSObject::ForceSetPrototype(string_iterator_prototype, iterator_prototype);

[Benedikt Meurer, 2016/09/15 17:34:33]

Not sure about this ForceSetPrototype. We seem to just use
JSObject::SetPrototype elsewehere in this file.

[caitp, 2016/09/15 19:02:39]

was copy/pasted from one of the Map or Set iterator setup code. I'm not sure
this is actually the right place for this block to live at all, it seems out of
place.

Will change it to SetPrototype and see if that breaks anything, anyway.
(actually, after uploading the patch, I may have forgotten to change it to
SetPrototype, but I'll wait for the tryjob to see if there are other failures
before changing it)

+
+    JSObject::AddProperty(
+        string_iterator_prototype, factory()->to_string_tag_symbol(),
+        factory()->NewStringFromAsciiChecked("String Iterator"),
+        static_cast<PropertyAttributes>(DONT_ENUM | READ_ONLY));
+
+    InstallFunction(string_iterator_prototype, "next", JS_OBJECT_TYPE,
+                    JSObject::kHeaderSize, MaybeHandle<JSObject>(),
+                    Builtins::kStringIteratorNext);
+
+    Handle<JSFunction> string_iterator_function = CreateFunction(
+        isolate(), factory()->NewStringFromAsciiChecked("StringIterator"),
+        JS_STRING_ITERATOR_TYPE, JSStringIterator::kSize,
+        string_iterator_prototype, Builtins::kIllegal);
+    native_context()->set_string_iterator_map(
+        string_iterator_function->initial_map());
+  }
+
   return true;
 }
 
 
 bool Genesis::InstallExperimentalNatives() {
   static const char* harmony_explicit_tailcalls_natives[] = {nullptr};
   static const char* harmony_tailcalls_natives[] = {nullptr};
   static const char* harmony_sharedarraybuffer_natives[] = {
       "native harmony-atomics.js", NULL};
   static const char* harmony_simd_natives[] = {"native harmony-simd.js",
@@ skipping 749 matching lines @@
 }
 
 
 // Called when the top-level V8 mutex is destroyed.
 void Bootstrapper::FreeThreadResources() {
   DCHECK(!IsActive());
 }
 
 }  // namespace internal
 }  // namespace v8