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.

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

convert RegEx properties #1183

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Feb 15, 2022

Implements the Regex part of #963

@@ -109,6 +109,12 @@

private static final int ANCHOR_BOL = -2;

private static final SymbolKey GET_FLAGS = new SymbolKey("[Symbol.getFlags]");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a Symbol? Symbol is written as [ @@match ].
https://262.ecma-international.org/12.0/#sec-get-regexp.prototype.flags

@@ -126,6 +132,36 @@ public static void init(Context cx, Scriptable scope, boolean sealed) {

ctor.setImmunePrototypeProperty(proto);

ScriptableObject desc = (ScriptableObject) cx.newObject(scope);
Copy link
Contributor

Choose a reason for hiding this comment

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

(I don't know much about this area, but...) Could you write simply using Lambda?

constructor.defineConstructorMethod(
scope, "resolve", 1, NativePromise::resolve, DONTENUM, DONTENUM | READONLY);
constructor.defineConstructorMethod(
scope, "reject", 1, NativePromise::reject, DONTENUM, DONTENUM | READONLY);
constructor.defineConstructorMethod(
scope, "all", 1, NativePromise::all, DONTENUM, DONTENUM | READONLY);
constructor.defineConstructorMethod(
scope, "allSettled", 1, NativePromise::allSettled, DONTENUM, DONTENUM | READONLY);
constructor.defineConstructorMethod(
scope, "race", 1, NativePromise::race, DONTENUM, DONTENUM | READONLY);
ScriptableObject speciesDescriptor = (ScriptableObject) cx.newObject(scope);
ScriptableObject.putProperty(speciesDescriptor, "enumerable", false);
ScriptableObject.putProperty(speciesDescriptor, "configurable", true);
ScriptableObject.putProperty(
speciesDescriptor,
"get",
new LambdaFunction(
scope,
"get [Symbol.species]",
0,
(Context lcx, Scriptable lscope, Scriptable thisObj, Object[] args) ->
constructor));
constructor.defineOwnProperty(cx, SymbolKey.SPECIES, speciesDescriptor, false);
constructor.definePrototypeMethod(
scope,
"then",
2,
(Context lcx, Scriptable lscope, Scriptable thisObj, Object[] args) -> {
NativePromise self =
LambdaConstructor.convertThisObject(thisObj, NativePromise.class);
return self.then(lcx, lscope, constructor, args);
},
DONTENUM,
DONTENUM | READONLY);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will check.....

prototype/multiline/this-val-regexp-prototype.js
prototype/source/cross-realm.js {unsupported: [cross-realm]}
prototype/source/length.js
prototype/source/name.js
prototype/source/prop-desc.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why this one doesn't pass yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strange will have a look

@rbri
Copy link
Collaborator Author

rbri commented Feb 17, 2022

This symbol seem to be too hacky, but the id scriptable object is a nightmare for this kind of properties. I have to digg deeper into the mud here. - will require some more days.

@p-bakker
Copy link
Collaborator

p-bakker commented Mar 2, 2022

@rbri just wondering how this PR is coming along, as I'm working on implementing the unicode flag and while analysing failing tests related to the unicode property, some are due me currently having added the unicode property as a data property instead of an accessor property

prototype/source/prop-desc.js
prototype/source/this-val-regexp-prototype.js
prototype/source/value-empty.js
prototype/source/value-line-terminator.js
prototype/source/value-u.js
prototype/sticky/cross-realm.js {unsupported: [cross-realm]}
prototype/sticky/length.js
prototype/sticky/name.js
prototype/sticky/prop-desc.js
prototype/sticky/this-val-regexp-prototype.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test is dependant on the sticky property being implemented as an accessor property. However, it looks like your changes haven't caused it to pass yet.

Not sure if that is due to the accessor property impl or if something else, possibly unrelated, is going on

@p-bakker
Copy link
Collaborator

@rbri just wondering how this PR is coming along

@p-bakker p-bakker added this to the Release 1.7.15 milestone Apr 14, 2022
@p-bakker p-bakker removed this from the Release 1.7.15 milestone May 21, 2024
@p-bakker
Copy link
Collaborator

@rbri any chance for a quick update on this one?

Is getting these types of properties properly implemented in IdScriptables maybe dead in the water completely and we should look at converting relevant implementations into lambda's instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants