Revise Symbol description getter methods

Signed-off-by: HyukWoo Park <[email protected]>
Samsung · Jan 8, 2024 · 906fe63 · 906fe63
1 parent 33182d2
commit 906fe63
Show file tree

Hide file tree

Showing 14 changed files with 81 additions and 68 deletions.
diff --git a/build/android/escargot/src/androidTest/java/com/samsung/lwe/escargot/EscargotTest.java b/build/android/escargot/src/androidTest/java/com/samsung/lwe/escargot/EscargotTest.java
@@ -648,18 +648,18 @@ public void symbolValueTest() {
         Context context = Context.create(vmInstance);
 
         JavaScriptSymbol symbol = JavaScriptSymbol.create(Optional.empty());
-        assertFalse(symbol.description().isPresent());
+        assertTrue(symbol.descriptionValue().isUndefined());
         assertTrue(symbol.symbolDescriptiveString().toJavaString().equals("Symbol()"));
         printNegativeTC("symbol null test 1");
 
         symbol = JavaScriptSymbol.create((Optional<JavaScriptString>) null);
-        assertFalse(symbol.description().isPresent());
+        assertTrue(symbol.descriptionValue().isUndefined());
         assertTrue(symbol.symbolDescriptiveString().toJavaString().equals("Symbol()"));
         printNegativeTC("symbol null test 2");
 
         symbol = JavaScriptSymbol.create(Optional.of(JavaScriptString.create("foobar")));
-        assertTrue(symbol.description().isPresent());
-        assertTrue(symbol.description().get().toJavaString().equals("foobar"));
+        assertFalse(symbol.descriptionValue().isUndefined());
+        assertTrue(symbol.descriptionString().toJavaString().equals("foobar"));
         assertFalse(symbol.equalsTo(context, JavaScriptSymbol.create(Optional.of(JavaScriptString.create("foobar")))).get().booleanValue());
         printNegativeTC("symbol null test 4");
 
@@ -1927,4 +1927,4 @@ public Optional<JavaScriptValue> callback(Context context, JavaScriptValue recei
         vmInstance = null;
         finalizeEngine();
     }
-}
+}
diff --git a/build/android/escargot/src/main/cpp/JNISymbol.cpp b/build/android/escargot/src/main/cpp/JNISymbol.cpp
@@ -56,10 +56,18 @@ Java_com_samsung_lwe_escargot_JavaScriptSymbol_fromGlobalSymbolRegistry(JNIEnv*
 
 extern "C"
 JNIEXPORT jobject JNICALL
-Java_com_samsung_lwe_escargot_JavaScriptSymbol_description(JNIEnv* env, jobject thiz)
+Java_com_samsung_lwe_escargot_JavaScriptSymbol_descriptionString(JNIEnv* env, jobject thiz)
 {
-    auto desc = unwrapValueRefFromValue(env, env->GetObjectClass(thiz), thiz)->asSymbol()->description();
-    return nativeOptionalValueIntoJavaOptionalValue(env, desc);
+    auto desc = unwrapValueRefFromValue(env, env->GetObjectClass(thiz), thiz)->asSymbol()->descriptionString();
+    return createJavaObjectFromValue(env, desc);
+}
+
+extern "C"
+JNIEXPORT jobject JNICALL
+Java_com_samsung_lwe_escargot_JavaScriptSymbol_descriptionValue(JNIEnv* env, jobject thiz)
+{
+    auto desc = unwrapValueRefFromValue(env, env->GetObjectClass(thiz), thiz)->asSymbol()->descriptionValue();
+    return createJavaObjectFromValue(env, desc);
 }
 
 extern "C"
@@ -68,4 +76,4 @@ Java_com_samsung_lwe_escargot_JavaScriptSymbol_symbolDescriptiveString(JNIEnv* e
 {
     auto desc = unwrapValueRefFromValue(env, env->GetObjectClass(thiz), thiz)->asSymbol()->symbolDescriptiveString();
     return createJavaObjectFromValue(env, desc);
-}
+}
diff --git a/build/android/escargot/src/main/java/com/samsung/lwe/escargot/JavaScriptSymbol.java b/build/android/escargot/src/main/java/com/samsung/lwe/escargot/JavaScriptSymbol.java
@@ -9,7 +9,8 @@ protected JavaScriptSymbol(long nativePointer)
     }
 
     native static public JavaScriptSymbol create(Optional<JavaScriptString> value);
-    native public Optional<JavaScriptString> description();
+    native public JavaScriptString descriptionString();
+    native public JavaScriptValue descriptionValue();
     native public JavaScriptString symbolDescriptiveString();
 
     /**

diff --git a/src/api/EscargotPublic.cpp b/src/api/EscargotPublic.cpp
@@ -784,12 +784,14 @@ SymbolRef* SymbolRef::fromGlobalSymbolRegistry(VMInstanceRef* vm, StringRef* des
     return toRef(Symbol::fromGlobalSymbolRegistry(toImpl(vm), toImpl(desc)));
 }
 
-OptionalRef<StringRef> SymbolRef::description()
+StringRef* SymbolRef::descriptionString()
 {
-    if (toImpl(this)->description().hasValue()) {
-        return toRef(toImpl(this)->description().value());
-    }
-    return nullptr;
+    return toRef(toImpl(this)->descriptionString());
+}
+
+ValueRef* SymbolRef::descriptionValue()
+{
+    return toRef(toImpl(this)->descriptionValue());
 }
 
 StringRef* SymbolRef::symbolDescriptiveString()
@@ -3823,7 +3825,7 @@ StringRef* StringObjectRef::primitiveValue()
 
 SymbolObjectRef* SymbolObjectRef::create(ExecutionStateRef* state)
 {
-    return toRef(new SymbolObject(*toImpl(state), new Symbol(String::emptyString)));
+    return toRef(new SymbolObject(*toImpl(state), new Symbol()));
 }
 
 void SymbolObjectRef::setPrimitiveValue(ExecutionStateRef* state, SymbolRef* value)

diff --git a/src/api/EscargotPublic.h b/src/api/EscargotPublic.h
@@ -1162,7 +1162,8 @@ class ESCARGOT_EXPORT SymbolRef : public PointerValueRef {
 public:
     static SymbolRef* create(OptionalRef<StringRef> desc);
     static SymbolRef* fromGlobalSymbolRegistry(VMInstanceRef* context, StringRef* desc); // this is same with Symbol.for
-    OptionalRef<StringRef> description();
+    StringRef* descriptionString();
+    ValueRef* descriptionValue();
     StringRef* symbolDescriptiveString();
 };
 

diff --git a/src/builtins/BuiltinSymbol.cpp b/src/builtins/BuiltinSymbol.cpp
@@ -33,14 +33,16 @@ static Value builtinSymbolConstructor(ExecutionState& state, Value thisValue, si
     if (newTarget.hasValue()) {
         ErrorObject::throwBuiltinError(state, ErrorCode::TypeError, "illegal constructor Symbol");
     }
-    Optional<String*> descString = nullptr;
+
     // If description is undefined, let descString be undefined.
-    if (!(argc == 0 || argv[0].isUndefined())) {
+    // (NOTE) we represent `undefined` as nullptr here
+    String* descStr = nullptr;
+    if (argc > 0 && !argv[0].isUndefined()) {
         // Else, let descString be ? ToString(description).
-        descString = argv[0].toString(state);
+        descStr = argv[0].toString(state);
     }
-    // Return a new unique Symbol value whose [[Description]] value is descString.
-    return new Symbol(descString);
+    // Return a new Symbol whose [[Description]] is descString.
+    return new Symbol(descStr);
 }
 
 #define RESOLVE_THIS_BINDING_TO_SYMBOL(NAME, OBJ, BUILT_IN_METHOD)                                                                                                                                                                                         \
@@ -88,35 +90,15 @@ static Value builtinSymbolKeyFor(ExecutionState& state, Value thisValue, size_t
         ErrorObject::throwBuiltinError(state, ErrorCode::TypeError, ErrorObject::Messages::GlobalObject_IllegalFirstArgument);
     }
     Symbol* sym = argv[0].asSymbol();
-    // For each element e of the GlobalSymbolRegistry List (see 19.4.2.1),
-    auto& list = state.context()->vmInstance()->globalSymbolRegistry();
-    for (size_t i = 0; i < list.size(); i++) {
-        // If SameValue(e.[[Symbol]], sym) is true, return e.[[Key]].
-        if (list[i].symbol == sym) {
-            return list[i].key.value();
-        }
-    }
-    // Assert: GlobalSymbolRegistry does not currently contain an entry for sym.
-    // Return undefined.
-    return Value();
+    return Symbol::keyForSymbol(state.context()->vmInstance(), sym);
 }
 
 static Value builtinSymbolDescriptionGetter(ExecutionState& state, Value thisValue, size_t argc, Value* argv, Optional<Object*> newTarget)
 {
     if (thisValue.isSymbol()) {
-        if (!thisValue.asSymbol()->description().hasValue()) {
-            return Value();
-        }
-
-        return thisValue.asSymbol()->description().value();
-
+        return thisValue.asSymbol()->descriptionValue();
     } else if (thisValue.isObject() && thisValue.asObject()->isSymbolObject()) {
-        if (!thisValue.asObject()->asSymbolObject()->primitiveValue()->description().hasValue()) {
-            return Value();
-        }
-
-        return thisValue.asObject()->asSymbolObject()->primitiveValue()->description().value();
-
+        return thisValue.asObject()->asSymbolObject()->primitiveValue()->descriptionValue();
     } else {
         ErrorObject::throwBuiltinError(state, ErrorCode::TypeError, "getter called on non-Symbol object");
     }

diff --git a/src/debugger/Debugger.cpp b/src/debugger/Debugger.cpp
@@ -602,11 +602,7 @@ static void sendProperty(DebuggerRemote* debugger, ExecutionState* state, Atomic
         } else {
             type = DebuggerRemote::ESCARGOT_VARIABLE_SYMBOL;
             Symbol* symbol = value.asSymbol();
-
-            valueStr = String::emptyString;
-            if (symbol->description().hasValue()) {
-                valueStr = symbol->description().value();
-            }
+            valueStr = symbol->descriptionString();
         }
 
         if (valueStr->length() >= ESCARGOT_DEBUGGER_MAX_VARIABLE_LENGTH) {

diff --git a/src/interpreter/ByteCodeInterpreter.cpp b/src/interpreter/ByteCodeInterpreter.cpp
@@ -4165,10 +4165,10 @@ static Value createObjectPropertyFunctionName(ExecutionState& state, const Value
     StringBuilder builder;
     if (name.isSymbol()) {
         builder.appendString(prefix);
-        if (name.asSymbol()->description().hasValue()) {
+        if (name.asSymbol()->descriptionString()->length()) {
             // add symbol name if it is not an empty symbol
             builder.appendString("[");
-            builder.appendString(name.asSymbol()->description().value());
+            builder.appendString(name.asSymbol()->descriptionString());
             builder.appendString("]");
         }
     } else {

diff --git a/src/runtime/Symbol.cpp b/src/runtime/Symbol.cpp
@@ -24,8 +24,19 @@
 
 namespace Escargot {
 
+String* Symbol::descriptionString() const
+{
+    return m_description ? const_cast<String*>(m_description.value()) : String::emptyString;
+}
+
+Value Symbol::descriptionValue() const
+{
+    return m_description ? m_description.value() : Value();
+}
+
 Symbol* Symbol::fromGlobalSymbolRegistry(VMInstance* vm, String* stringKey)
 {
+    ASSERT(!!stringKey);
     // Let stringKey be ? ToString(key).
     // For each element e of the GlobalSymbolRegistry List,
     auto& list = vm->globalSymbolRegistry();
@@ -47,12 +58,26 @@ Symbol* Symbol::fromGlobalSymbolRegistry(VMInstance* vm, String* stringKey)
     return newSymbol;
 }
 
+Value Symbol::keyForSymbol(VMInstance* vm, Symbol* sym)
+{
+    ASSERT(!!sym);
+    auto& list = vm->globalSymbolRegistry();
+
+    for (size_t i = 0; i < list.size(); i++) {
+        // If SameValue(e.[[Symbol]], sym) is true, return e.[[Key]].
+        if (list[i].symbol == sym) {
+            ASSERT(!!list[i].key);
+            return list[i].key;
+        }
+    }
+    return Value();
+}
+
 String* Symbol::symbolDescriptiveString() const
 {
     StringBuilder sb;
     sb.appendString("Symbol(");
-    if (description().hasValue())
-        sb.appendString(description().value());
+    sb.appendString(descriptionString());
     sb.appendString(")");
     return sb.finalize();
 }

diff --git a/src/runtime/Symbol.h b/src/runtime/Symbol.h
@@ -49,10 +49,8 @@ class Symbol : public PointerValue {
     {
     }
 
-    Optional<String*> description() const
-    {
-        return m_description;
-    }
+    String* descriptionString() const;
+    Value descriptionValue() const;
 
     SymbolFinalizerData* finalizerData() const
     {
@@ -63,6 +61,7 @@ class Symbol : public PointerValue {
     String* symbolDescriptiveString() const;
 
     static Symbol* fromGlobalSymbolRegistry(VMInstance* vm, String* stringKey);
+    static Value keyForSymbol(VMInstance* vm, Symbol* sym);
 
 private:
     SymbolFinalizerData* ensureFinalizerData();
@@ -71,7 +70,7 @@ class Symbol : public PointerValue {
     void tryToShrinkFinalizers();
 
     size_t m_typeTag;
-    Optional<String*> m_description;
+    Optional<String*> m_description; // nullptr of desc represents `undefined`
     SymbolFinalizerData* m_finalizerData; // handle finalizer data of Symbol
 };
 } // namespace Escargot

diff --git a/src/runtime/VMInstance.h b/src/runtime/VMInstance.h
@@ -65,7 +65,7 @@ struct GlobalSymbols {
 };
 
 struct GlobalSymbolRegistryItem {
-    Optional<String*> key;
+    String* key;
     Symbol* symbol;
 };
 

diff --git a/src/runtime/Value.cpp b/src/runtime/Value.cpp
@@ -114,10 +114,9 @@ String* Value::toStringSlowCase(ExecutionState& ec) const // $7.1.12 ToString
         ASSERT_NOT_REACHED();
     } else if (isBigInt()) {
         return asBigInt()->toString();
-    } else {
-        return toPrimitive(ec, PreferString).toString(ec);
     }
-    return nullptr;
+
+    return toPrimitive(ec, PreferString).toString(ec);
 }
 
 Object* Value::toObjectSlowCase(ExecutionState& state) const // $7.1.13 ToObject

diff --git a/src/runtime/serialization/Serializer.cpp b/src/runtime/serialization/Serializer.cpp
@@ -46,8 +46,8 @@ std::unique_ptr<SerializedValue> Serializer::serialize(const Value& value)
     } else if (value.isBigInt()) {
         return std::unique_ptr<SerializedValue>(new SerializedBigIntValue(value.asBigInt()->toString()->toNonGCUTF8StringData()));
     } else if (value.isSymbol()) {
-        if (value.asSymbol()->description()) {
-            return std::unique_ptr<SerializedValue>(new SerializedSymbolValue(value.asSymbol()->description()->toNonGCUTF8StringData()));
+        if (value.asSymbol()->descriptionString()->length()) {
+            return std::unique_ptr<SerializedValue>(new SerializedSymbolValue(value.asSymbol()->descriptionString()->toNonGCUTF8StringData()));
         } else {
             return std::unique_ptr<SerializedValue>(new SerializedSymbolValue());
         }

diff --git a/test/cctest/testapi.cpp b/test/cctest/testapi.cpp
@@ -1874,7 +1874,7 @@ TEST(EnumerateObjectOwnProperties, Basic1)
         EXPECT_TRUE(indexes[1] == 10);
         EXPECT_TRUE(strings->at(0)->equalsTo(state, StringRef::createFromASCII("a_enum")));
         EXPECT_TRUE(strings->at(1)->equalsTo(state, StringRef::createFromASCII("-20")));
-        EXPECT_TRUE(symbols->at(0)->asSymbol()->description()->equalsTo(state, StringRef::createFromASCII("sym_enum")));
+        EXPECT_TRUE(symbols->at(0)->asSymbol()->descriptionString()->equalsTo(state, StringRef::createFromASCII("sym_enum")));
 
         return ValueRef::createUndefined();
     });
@@ -2318,7 +2318,7 @@ TEST(Serializer, Basic7)
     SerializerRef::serializeInto(v1, ostream);
     std::istringstream istream(ostream.str());
     ValueRef* v2 = SerializerRef::deserializeFrom(g_context.get(), istream);
-    EXPECT_FALSE(v2->asSymbol()->description().hasValue());
+    EXPECT_TRUE(v2->asSymbol()->descriptionValue()->isUndefined());
 }
 
 TEST(Serializer, Basic8)
@@ -2328,7 +2328,7 @@ TEST(Serializer, Basic8)
     SerializerRef::serializeInto(v1, ostream);
     std::istringstream istream(ostream.str());
     ValueRef* v2 = SerializerRef::deserializeFrom(g_context.get(), istream);
-    EXPECT_TRUE(v2->asSymbol()->description().value()->equalsWithASCIIString("asdfasdfasdf", 12));
+    EXPECT_TRUE(v2->asSymbol()->descriptionString()->equalsWithASCIIString("asdfasdfasdf", 12));
 }
 
 TEST(Serializer, Basic9)