-
Notifications
You must be signed in to change notification settings - Fork 684
Fix the indexing of Array builtin functions. #196
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
Fix the indexing of Array builtin functions. #196
Conversation
* @return uint32_t - the normalized value of the index | ||
*/ | ||
uint32_t | ||
ecma_number_helper_index_normalize (ecma_number_t num, /**< index */ |
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.
Could you, please, move the function to jerry-core/ecma/builtin-objects/ecma-builtin-helpers.cpp, as this is a helper for built-ins implementation?
Could you also, please, add more detailed comment about what is normalized index and "See also" section, listing built-in routines that use the helper?
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.
Of course, I've updated the PR.
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.
Thank you.
@@ -892,41 +893,15 @@ ecma_builtin_array_prototype_object_index_of (ecma_value_t this_arg, /**< this a | |||
/* 5. */ | |||
ECMA_OP_TO_NUMBER_TRY_CATCH (arg_from_idx, arg2, ret_value); | |||
|
|||
int32_t from_idx_int = ecma_number_to_int32 (arg_from_idx); | |||
uint32_t k = ecma_builtin_helper_array_index_normalize (arg_from_idx, len); |
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.
Rename k
to smth more meaningful, please.
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.
Fixed.
The string prototype was also modified in this PR, but there is no test for it. Also the title of the commit/PR does not reflect that the string was also modified. |
Yes, because this bug only occured in Array.prototype, in String.prototype I just replaced the clamping part to call the clamper function. This replace didn't changed the functionality of |
@bzsolt ok, thanks for the info. |
lgtm |
/* 7. */ | ||
else | ||
{ | ||
int_from_idx = - int_from_idx; |
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.
-int_from_idx
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 am a little bit confused, because I used the requested style in a previous
commit, but it was not proper.
So which is the good one? Is there a coding style for Jerry somewhere?
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.
Ah, my bad. Last time I was wrong.
Our current style should be the following:
int a = b - c; // subtraction
int a = -a; // negate
PS Actually this checks should be implemented as vera++ rule, but some of rules are missing right now.
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.
Thanks for the explanation, I've fixed it.
|
The index-dependant builtins didn't handle correctly the positive Infinity value. JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]
Rebased & merged: 3f28cb3 |
The index-dependant builtins didn't handle correctly the positive Infinity value.
JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]