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

Es2022 at method #1289

Merged
merged 17 commits into from
May 31, 2023
Merged

Es2022 at method #1289

merged 17 commits into from
May 31, 2023

Conversation

JohnBain
Copy link
Contributor

@JohnBain JohnBain commented Dec 6, 2022

Addresses issue #1051 , adding .at function to the indexed data types Array, TypedArray and String.

@p-bakker
Copy link
Collaborator

p-bakker commented Dec 8, 2022

Some of the other NativeArray methods implementations check whether the instance being operated on is an instance of NativeArray and then whether the instance.denseOnly flag is set to true.

Not sure if such a branch in the code would be applicable for the .at() method as well. To my knowledge the .denseOnly flag is related to a performance optimize

Copy link
Collaborator

@rbri rbri left a comment

Choose a reason for hiding this comment

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

can you please move your tests to org.mozilla.javascript.tests.es2022

@JohnBain
Copy link
Contributor Author

can you please move your tests to org.mozilla.javascript.tests.es2022

Done - 8ea870b

@rbri
Copy link
Collaborator

rbri commented Apr 11, 2023

@JohnBain thanks, hopefully i find some time today to have a mor detailed look at this

@@ -1962,6 +1970,18 @@ private static Object js_copyWithin(
return thisObj;
}

private static Object js_at(Context cx, Scriptable scope, Scriptable thisObj, Object[] args) {
Scriptable o = ScriptRuntime.toObject(cx, scope, thisObj);
Object targetArg = (args.length >= 1) ? args[0] : Undefined.instance;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ScriptRuntime has overloaded variants of toInteger that might be a better fit here, like public static double toNumber(Object[] args, int index)

@@ -254,6 +254,18 @@ private Object js_subarray(Context cx, Scriptable scope, int s, int e) {
new Object[] {arrayBuffer, Integer.valueOf(byteOff), Integer.valueOf(len)});
}

private Object js_at(Context cx, Scriptable scope, Scriptable thisObj, Object[] args) {
Object targetArg = (args.length >= 1) ? args[0] : Undefined.instance;
int relativeIndex = (int) ScriptRuntime.toInteger(targetArg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar comment as the one above

@gbrail
Copy link
Collaborator

gbrail commented Apr 25, 2023

This looks like a pretty complete and self-contained PR. We have two outstanding comments that could make the code a bit simpler (but I'm not actually 100% sure of that). Would you all be OK if we merged this now?

@rbri rbri merged commit 4f531c8 into mozilla:master May 31, 2023
4 checks passed
@rbri
Copy link
Collaborator

rbri commented May 31, 2023

@p-bakker will have a look at your comments in the next step....

@rbri
Copy link
Collaborator

rbri commented May 31, 2023

@p-bakker #1331 brings the code in sync with js_fill impl regarding parameter handling

@rbri
Copy link
Collaborator

rbri commented May 31, 2023

@JohnBain thanks for the pr

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.

Support ES2022 .at() on indexables (Array, String, etc)
5 participants