Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement Legacy RegExp features #832

Merged
merged 1 commit into from
Feb 16, 2021
Merged

Implement Legacy RegExp features #832

merged 1 commit into from
Feb 16, 2021

Conversation

kisbg
Copy link
Contributor

@kisbg kisbg commented Feb 3, 2021

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

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

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
Copy link
Contributor Author

kisbg commented Feb 8, 2021

@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
Copy link
Contributor

@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
Copy link
Contributor Author

kisbg commented Feb 8, 2021

@clover2123 Of course!

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

kisbg commented Feb 9, 2021

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

clover2123
clover2123 requested changes Feb 10, 2021
View reviewed changes
src/runtime/RegExpObject.h Outdated

void setLegacyFeaturesEnabled(bool is_enabled)
{
m_legacyFeaturesEnabled = is_enabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

static void InvalidateLegacyRegExpStaticProperties(ExecutionState& state)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/runtime/GlobalObjectBuiltinRegExp.cpp Outdated
Comment on lines 676 to 671
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you call setLegacyRegExpStaticProperty here?

clover2123
clover2123 requested changes Feb 10, 2021
View reviewed changes
src/runtime/GlobalObjectBuiltinRegExp.cpp Outdated
}

{
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/runtime/GlobalObjectBuiltinRegExp.cpp Outdated
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@clover2123
Copy link
Contributor

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 kisbg force-pushed the newFeature-legacyRegExp branch from fe415e5 to c8bb1d7 Compare February 10, 2021 08:23
@kisbg
Copy link
Contributor Author

kisbg commented Feb 11, 2021

@clover2123 I updated the pull request!

clover2123
clover2123 reviewed Feb 15, 2021
View reviewed changes
src/runtime/GlobalObjectBuiltinRegExp.cpp Outdated
Comment on lines 651 to 654
ErrorObject::throwBuiltinError(state, ErrorObject::TypeError, "thisValue is not RegExp"); \
} \
if (stringView.isEmpty()) { \
ErrorObject::throwBuiltinError(state, ErrorObject::TypeError, "Value is empty"); \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/runtime/RegExpObject.cpp Outdated
Comment on lines 463 to 467
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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

kisbg commented Feb 15, 2021

@clover2123 Thank you for the review!

@clover2123
Copy link
Contributor

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

Implement Legacy RegExp features
2ddd965 
Signed-off-by: bence gabor kis <[email protected]>
@kisbg kisbg force-pushed the newFeature-legacyRegExp branch from 24d4079 to 2ddd965 Compare February 15, 2021 11:35
@kisbg
Copy link
Contributor Author

kisbg commented Feb 15, 2021

Sorry for the inconvenience!

clover2123
clover2123 approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ksh8281 ksh8281 merged commit 560f1f2 into Samsung:master Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clover2123 clover2123 clover2123 approved these changes

@ksh8281 ksh8281 ksh8281 approved these changes

@bbrto21 bbrto21 Awaiting requested review from bbrto21 bbrto21 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kisbg @clover2123 @ksh8281