-
Notifications
You must be signed in to change notification settings - Fork 851
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
Exponentiation Operator #838
Conversation
if (doubleLen > NativeNumber.MAX_SAFE_INTEGER) { | ||
if (throwIfTooLarge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is probably the policy.
- get length
- Up to 53 ** 2 - 1
- https://262.ecma-international.org/11.0/#sec-tolength
- set length
- Up to 31 ** 2 - 1
- https://262.ecma-international.org/11.0/#sec-arraycreate
switch (tt) { | ||
case Token.EXP: | ||
if (pn instanceof UnaryExpression) { | ||
reportError("msg.no.unary.expr.on.left.exp", AstNode.operatorToString(pn.getType())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
js> -1 ** 2
js: "<stdin>", line 2: "-" is not allowed on the left-hand side of "**".
js: -1 ** 2
js: .....^
The error appears to be in the wrong position.
I tried void reportError(String messageId, int position, int length)
, but it didn't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the shell does this for everything -- so fixing this shouldn't be in scope for this PR assuming that errors loaded from a regular file show up on the right line.
This change looks OK to me. I think that you've handled the various limits on the lengths properly according to the spec that you linked. I see that you have a "TODO" about an error format -- do you want to fix that first? |
Do you mean this? rhino/src/org/mozilla/javascript/NativeArray.java Lines 1933 to 1934 in c61469d
I think it's probably rare to have a problem, so I'd like to have a next time. https://262.ecma-international.org/11.0/#sec-array.prototype.filter
Not here
|
@@ -1914,7 +1929,13 @@ private static Object iterativeMethod(Context cx, IdFunctionObject idFunctionObj | |||
requireObjectCoercible(cx, o, idFunctionObject); | |||
} | |||
|
|||
long length = getLengthProperty(cx, o, id == Id_map); | |||
long length = getLengthProperty(cx, o); | |||
// TODO: In the case of Id_filter, CreateDataPropertyOrThrow throws a TypeError instead of ArraySpeciesCreate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I'm asking about -- previously, we only threw an error in the case of Id_map. Is there a reason to also throw it for Id_filter too? If this is a change then you know I'm going to ask if we should check the language version too since if we didn't previously throw in this situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
25f317e
Sorry. There should have been a slow test, but I checked and it didn't slow down without throws an error.
Sorry I wasn't clear -- the second TODO was about exception handling, so please check out my comments. |
Thanks for pulling this together! |
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Exponentiation
#817 (comment)
Although not directly related to this feature, the test for checking the limit of the array length became very time consuming, so it has been fixed.