Update NativeArray for toReversed() #1414
Conversation
|
I'd set a breakpoint early in the stacktrace you're getting and then see what is the difference between a call to [1,2,3].reverse() and [1,2,3].toReversed(). If you do, I think you'll find that you forgot to update MAX_PROTOTYPE_ID to the new value |
cool, thanks. I know it is a basic question, but how to debug from the shell? Is it this one: https://rhino.github.io/tools/debugger/ Says there is no breakpoint available? |
|
That's the JavaScript Debugger. In your case as you're developing Rhino itself, in Java, you'll need a Java debugger. I personally use Eclipse and run the rhino shell from source inside Eclipse, but you can use whatever Java development environment you like |
Ta, able to debug now and no more the error! Appreciate it |
| private static Scriptable js_toReversed( | ||
| Context cx, Scriptable scope, Scriptable thisObj) { | ||
| Scriptable o = ScriptRuntime.toObject(cx, scope, thisObj); | ||
| long len = (getLengthProperty(cx, o) > Integer.MAX_VALUE) ? 0 : getLengthProperty(cx, o); |
There was a problem hiding this comment.
i guess getLengthProperty(cx, o) is expensive, maybe calling this only once is a good idea
| @@ -2729,7 +2753,8 @@ protected int findPrototypeId(String s) { | |||
| Id_at = 32, | |||
| Id_flat = 33, | |||
| Id_flatMap = 34, | |||
| SymbolId_iterator = 35, | |||
| Id_toReversed =36, | |||
| @@ -2729,7 +2753,8 @@ protected int findPrototypeId(String s) { | |||
| Id_at = 32, | |||
| Id_flat = 33, | |||
| Id_flatMap = 34, | |||
| SymbolId_iterator = 35, | |||
There was a problem hiding this comment.
Leave this, just update MAX_PROTOTYPE_ID to be Id_toReversed
Hi there, I am looking the array-at.js file. Probably, a similar file for this new method? How do I run just the test js file? |
|
@pkkht you can have a look at the packages org.mozilla.javascript.tests.es2019, org.mozilla.javascript.tests.es2020, org.mozilla.javascript.tests.es2022. Check out e.g. org.mozilla.javascript.tests.es2020.TypedArrayAtTest for a sample. |
|
@pkkht I see you mentioned this PR being a draft, but it's not marked as such and it's referencing just the .toReversed() method What is your idea? Are you going to implement all the new methods of the Change Array by Copy proposal on NativeArray in this PR? Or are you planning to provide individual PRs for each method? |
|
As for the tests: the test262 testsuite for Array.prototype.toReversed() is (obviously) way more extensive than the tests you've added. This is not particularly an issue with the tests you've added, but it does make me wonder what we might be missing. One thing in particular with our NativeArray implementation is that it internally switches to another implementation if the length of an Array is over NativeArray.maximumInitialCapacity (also see the NativeArray.denseOnly flag. I wonder if we should have explicit tests with Arrays that go beyond this size I will see if I can expedite the work on #958 which hopefully would allow us to move to the latest or at least way more recent version of Test262 |
|
Okay, on a second thought, I will use the same PR for all the methods. I see there is more work in the tests.. so we can do altogether |
|
Why did the tests fail in the pipeline? |
| MAX_PROTOTYPE_ID = SymbolId_iterator; | ||
| Id_toReversed = 36, | ||
| SymbolId_iterator = 37, | ||
| MAX_PROTOTYPE_ID = Id_toReversed; | ||
| private static final int ConstructorId_join = -Id_join, |
There was a problem hiding this comment.
Id_flatMap = 34,
Id_toReversed = 35,
SymbolId_iterator = 36,
MAX_PROTOTYPE_ID = SymbolId_iterator;
|
@pkkht hi, wondering where you're at with this PR. Are you still working on it? |
|
Not as of now |
|
@pkkht note that Rhino now runs the Test262 test suite against a very recent version of test262, which includes all the tests for newer Array methods like .at() and .toReversed() So likely you can remove the testcases you added, because those would already be covered by test262 now |
|
Okay |
|
@pkkht why did you close this? Looks like the implementation of toReversed is still missing in Rhino and i think your PR will at least help to get this done. |
|
Sorry I got a bit confused with the recent comments. |
|
@rbri I am working on a PR for this (and the other new |
Hi,
I get the below error when testing the method manually from the console. I don't see NativeArray invoked in the exception stack trace. What's going wrong in the code? Any ideas please
js> const items = [1, 2, 3];
js> items
1,2,3
js> items.toReversed()
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: Index 70 out of bounds for length 70
at org.mozilla.javascript.IdScriptableObject$PrototypeValues.ensureId(IdScriptableObject.java:284)
at org.mozilla.javascript.IdScriptableObject$PrototypeValues.get(IdScriptableObject.java:162)
at org.mozilla.javascript.IdScriptableObject.get(IdScriptableObject.java:380)
at org.mozilla.javascript.ScriptableObject.getProperty(ScriptableObject.java:2037)
at org.mozilla.javascript.ScriptRuntime.getPropFunctionAndThisHelper(ScriptRuntime.java:2584)
at org.mozilla.javascript.ScriptRuntime.getPropFunctionAndThis(ScriptRuntime.java:2575)
at org.mozilla.javascript.optimizer.OptRuntime.callProp0(OptRuntime.java:71)
at org.mozilla.javascript.gen._stdin__3._c_script_0(:4)
at org.mozilla.javascript.gen._stdin__3.call()
at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:383)
at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3876)
at org.mozilla.javascript.gen._stdin__3.call()
at org.mozilla.javascript.gen._stdin__3.exec()
at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:482)
at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:180)
at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:98)
at org.mozilla.javascript.Context.call(Context.java:544)
at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:475)
at org.mozilla.javascript.tools.shell.Main.exec(Main.java:164)
at org.mozilla.javascript.tools.shell.Main.main(Main.java:142)