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.
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.
Of course, I've updated the PR.
egavrin
added
bug
| @@ -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.
Rename k to smth more meaningful, please.
|
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.
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.
Ah, my bad. Last time I was wrong.
Our current style should be the following:
int a = b - c; // subtraction
int a = -a; // negatePS Actually this checks should be implemented as vera++ rule, but some of rules are missing right now.
There was a problem hiding this comment.
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]