Implement Legacy RegExp features #832

kisbg · 2021-02-03T08:52:04Z

Signed-off-by: bence gabor kis [email protected]

clover2123 · 2021-02-05T01:43:38Z

I cannot find Legacy RegExp features in the current draft ECMA-262.
It seems that this spec is in the proposal stage and not yet written on the standard.
How about leaving this pr on pending until it is updated?

kisbg · 2021-02-08T07:31:13Z

@clover2123 Sorry for the late reply! This pull request based on this proposal documentation https://github.com/tc39/proposal-regexp-legacy-features.

Of course i can close this pull request and wait for a more official standard documentation.

clover2123 · 2021-02-08T08:15:00Z

@kisbg I've updated test262 to the latest version (#834)
After #834 landed, would you please check and re-run relevant test cases in the new test262?

kisbg · 2021-02-08T08:16:32Z

@clover2123 Of course!

kisbg · 2021-02-09T11:32:53Z

Updated the pull request and added the related test-cases in test262

clover2123 · 2021-02-10T03:22:40Z

src/runtime/RegExpObject.h

+
+    void setLegacyFeaturesEnabled(bool is_enabled)
+    {
+        m_legacyFeaturesEnabled = is_enabled;


Just a minor correction.
We generally don't use underscore(_) for parameter's name.
Please fix it to another name like isEnabled.

clover2123 · 2021-02-10T03:40:04Z

src/runtime/RegExpObject.cpp

@@ -457,6 +458,18 @@ void RegExpObject::createRegexMatchResult(ExecutionState& state, String* str, Re
    } while (testResult);
 }

+static void InvalidateLegacyRegExpStaticProperties(ExecutionState& state)
+{


We define each function name that starts with lower case like invalidate....

clover2123 · 2021-02-10T05:33:59Z

src/runtime/GlobalObjectBuiltinRegExp.cpp

 static Value builtinRegExpInputSetter(ExecutionState& state, Value thisValue, size_t argc, Value* argv, Optional<Object*> newTarget)
 {
+    if (!thisValue.isPointerValue() || thisValue.asPointerValue() != state.context()->globalObject()->regexp()) {
+        ErrorObject::throwBuiltinError(state, ErrorObject::TypeError, "thisValue is not RegExp");
+    }
    state.resolveCallee()->codeBlock()->context()->regexpStatus().input = argv[0].toString(state);


Why don't you call setLegacyRegExpStaticProperty here?

clover2123 · 2021-02-10T05:47:07Z

src/runtime/GlobalObjectBuiltinRegExp.cpp

    }

    {
        JSGetterSetter gs(
            new NativeFunctionObject(state, NativeFunctionInfo(strings->get, builtinRegExpLastMatchGetter, 0, NativeFunctionInfo::Strict)), Value(Value::EmptyValue));
-        m_regexp->defineOwnProperty(state, strings->lastMatch, ObjectPropertyDescriptor(gs, (ObjectPropertyDescriptor::PresentAttribute)(ObjectPropertyDescriptor::EnumerablePresent)));
-        m_regexp->defineOwnProperty(state, strings->$Ampersand, ObjectPropertyDescriptor(gs, (ObjectPropertyDescriptor::PresentAttribute)(ObjectPropertyDescriptor::NotPresent)));
+        new NativeFunctionObject(state, NativeFunctionInfo(strings->set, builtinRegExpLastMatchSetter, 1, NativeFunctionInfo::Strict));


This newly allocated NativeFunctionObject seems useless.
And there are other similar cases like this. Please check and remove it.

clover2123 · 2021-02-10T05:49:07Z

src/runtime/GlobalObjectBuiltinRegExp.cpp

+}
+
+static Value builtinRegExpLastMatchSetter(ExecutionState& state, Value thisValue, size_t argc, Value* argv, Optional<Object*> newTarget)
+{


According to the standard, RegExp.lastMatch does not have setter (getter only).
Remove this setter function and other setter functions too.

clover2123 · 2021-02-10T05:54:23Z

GetLegacyRegExpStaticProperty and SetLegacyRegExpStaticProperty are abstract operations that means we don't have to implement these functions exactly as spec.
How about simplify legacy property's getter/setter functions by using some macro instead of calling the above function in itself each time?

kisbg · 2021-02-11T08:09:14Z

@clover2123 I updated the pull request!

clover2123 · 2021-02-15T01:25:09Z

src/runtime/GlobalObjectBuiltinRegExp.cpp

+        ErrorObject::throwBuiltinError(state, ErrorObject::TypeError, "thisValue is not RegExp");                 \
+    }                                                                                                             \
+    if (stringView.isEmpty()) {                                                                                   \
+        ErrorObject::throwBuiltinError(state, ErrorObject::TypeError, "Value is empty");                          \


For error messages, how about exploiting existing messages defined in ErrorObject::Messages?
e.g) ErrorObject::Messages::GlobalObject_ThisNotRegExpObject for the first error

clover2123 · 2021-02-15T01:28:02Z

src/runtime/RegExpObject.cpp

+    state.resolveCallee()->codeBlock()->context()->regexpStatus().input = String::emptyString;
+    state.resolveCallee()->codeBlock()->context()->regexpStatus().lastMatch = StringView();
+    state.resolveCallee()->codeBlock()->context()->regexpStatus().lastParen = StringView();
+    state.resolveCallee()->codeBlock()->context()->regexpStatus().leftContext = StringView();
+    state.resolveCallee()->codeBlock()->context()->regexpStatus().rightContext = StringView();


It seems like that RegExpStatus could be accessed simply by state.context()->regexpStatus()

kisbg · 2021-02-15T07:16:00Z

@clover2123 Thank you for the review!

clover2123 · 2021-02-15T08:54:40Z

It seems that you have reverted the previous update.
Please check my comments once more.

Signed-off-by: bence gabor kis <[email protected]>

kisbg · 2021-02-15T11:36:23Z

Sorry for the inconvenience!

clover2123

LGTM

kisbg requested review from bbrto21, clover2123 and ksh8281 as code owners February 3, 2021 08:52

kisbg force-pushed the newFeature-legacyRegExp branch 2 times, most recently from aafd477 to e4c0aea Compare February 3, 2021 11:51

kisbg force-pushed the newFeature-legacyRegExp branch 5 times, most recently from e1e8aa6 to fe415e5 Compare February 9, 2021 10:02

clover2123 requested changes Feb 10, 2021

View reviewed changes

kisbg force-pushed the newFeature-legacyRegExp branch from fe415e5 to c8bb1d7 Compare February 10, 2021 08:23

clover2123 reviewed Feb 15, 2021

View reviewed changes

kisbg force-pushed the newFeature-legacyRegExp branch from c8bb1d7 to 24d4079 Compare February 15, 2021 06:59

Implement Legacy RegExp features

2ddd965

Signed-off-by: bence gabor kis <[email protected]>

kisbg force-pushed the newFeature-legacyRegExp branch from 24d4079 to 2ddd965 Compare February 15, 2021 11:35

clover2123 approved these changes Feb 16, 2021

View reviewed changes

ksh8281 approved these changes Feb 16, 2021

View reviewed changes

ksh8281 merged commit 560f1f2 into Samsung:master Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Legacy RegExp features #832

Implement Legacy RegExp features #832

kisbg commented Feb 3, 2021

clover2123 commented Feb 5, 2021

kisbg commented Feb 8, 2021

clover2123 commented Feb 8, 2021

kisbg commented Feb 8, 2021

kisbg commented Feb 9, 2021

clover2123 Feb 10, 2021

clover2123 Feb 10, 2021

clover2123 Feb 10, 2021

clover2123 Feb 10, 2021

clover2123 Feb 10, 2021

clover2123 commented Feb 10, 2021

kisbg commented Feb 11, 2021

clover2123 Feb 15, 2021

clover2123 Feb 15, 2021

kisbg commented Feb 15, 2021

clover2123 commented Feb 15, 2021

kisbg commented Feb 15, 2021

clover2123 left a comment

Implement Legacy RegExp features #832

Implement Legacy RegExp features #832

Conversation

kisbg commented Feb 3, 2021

clover2123 commented Feb 5, 2021

kisbg commented Feb 8, 2021

clover2123 commented Feb 8, 2021

kisbg commented Feb 8, 2021

kisbg commented Feb 9, 2021

clover2123 Feb 10, 2021

Choose a reason for hiding this comment

clover2123 Feb 10, 2021

Choose a reason for hiding this comment

clover2123 Feb 10, 2021

Choose a reason for hiding this comment

clover2123 Feb 10, 2021

Choose a reason for hiding this comment

clover2123 Feb 10, 2021

Choose a reason for hiding this comment

clover2123 commented Feb 10, 2021

kisbg commented Feb 11, 2021

clover2123 Feb 15, 2021

Choose a reason for hiding this comment

clover2123 Feb 15, 2021

Choose a reason for hiding this comment

kisbg commented Feb 15, 2021

clover2123 commented Feb 15, 2021

kisbg commented Feb 15, 2021

clover2123 left a comment

Choose a reason for hiding this comment