[builtins] Also simplify the Symbol constructor.

src/builtins.cc

Index: src/builtins.cc
diff --git a/src/builtins.cc b/src/builtins.cc
index ac6c9fa93a5d4d5fcba83fbd3e0e56a8276b813c..87f1037ceae6e5f5dd8569932bf94f71b1bcfef9 100644
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -1445,11 +1445,7 @@ BUILTIN(ArrayConcat) {
 }
 
 
-// -----------------------------------------------------------------------------
-//
-
-
-// 20.3.4.45 Date.prototype [ @@toPrimitive ] ( hint )
+// ES6 section 20.3.4.45 Date.prototype [ @@toPrimitive ] ( hint )
 BUILTIN(DateToPrimitive) {
   HandleScope scope(isolate);
   DCHECK_EQ(2, args.length());
@@ -1469,6 +1465,30 @@ BUILTIN(DateToPrimitive) {
 }
 
 
+// ES6 section 19.4.1.1 Symbol ( [ description ] ) for the [[Call]] case.
+BUILTIN(SymbolConstructor) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(2, args.length());
+  Handle<Symbol> result = isolate->factory()->NewSymbol();
+  Handle<Object> description = args.at<Object>(1);
+  if (!description->IsUndefined()) {
+    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, description,

[adamk, 2015/09/16 19:33:31]

I'd like to understand why this approach is better than the previous version.
Here the call stack is:

JS -> C++ -> JS

instead of the old

JS -> JS

I thought I'd understood that part of the point of these cleanups was to reduce
the hops between JS and C++, but that seems to be increased by this change.

If the main point of this change is to get rid of %_IsConstructCall(), you could
replace it with new.target instead, which in ES6 exposes the IsConstruct-ness of
a [[Call]] to author script.

[Benedikt Meurer, 2015/09/17 02:58:10]

The main point of these cleanups is to make it correct, and as you guessed,
%_IsConstructCall is buggy (in the case of TCO), and unfortunately the same
applies to new.target (currently). But in case of the Symbol constructor it was
never

 JS -> JS 

but always

 JS -> (JS ->) C++ -> JS

because it always ended up with a call to %CreateSymbol, and it was also using
the obsolete ToString implementation, which is why it wouldn't do the callback
from C++ to JS previously. So fixing all of that was easiest by just writing
everything in C++, and that's what we agreed on as baseline implementation. If
that ever becomes a bottleneck we can turn it into a native builtin.

+                                       Object::ToString(isolate, description));
+    result->set_name(*description);
+  }
+  return *result;
+}
+
+
+// ES6 section 19.4.1.1 Symbol ( [ description ] ) for the [[Construct]] case.
+BUILTIN(SymbolConstructor_ConstructStub) {
+  HandleScope scope(isolate);
+  THROW_NEW_ERROR_RETURN_FAILURE(
+      isolate, NewTypeError(MessageTemplate::kNotConstructor,
+                            isolate->factory()->Symbol_string()));
+}
+
+
 // -----------------------------------------------------------------------------
 // Throwers for restricted function properties and strict arguments object
 // properties