Skip to content

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

Closed
wants to merge 1 commit into from
Closed

Fix the indexing of Array builtin functions. #196

wants to merge 1 commit into from

Conversation

bzsolt
Copy link
Member

@bzsolt bzsolt commented Jun 16, 2015

The index-dependant builtins didn't handle correctly the positive Infinity value.

JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]

* @return uint32_t - the normalized value of the index
*/
uint32_t
ecma_number_helper_index_normalize (ecma_number_t num, /**< index */
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you.

@egavrin egavrin added bug Undesired behaviour ecma builtins Related to ECMA built-in routines labels Jun 16, 2015
@egavrin egavrin added this to the ECMA builtins milestone Jun 16, 2015
@@ -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);
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@galpeter
Copy link
Contributor

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.

@bzsolt
Copy link
Member Author

bzsolt commented Jun 17, 2015

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 String.prototype.slice().
In String.prototype.slice() the positive Infinity index was handled correctly.
The string-prototype-slice.js covers these cases, therefore I haven't added additional testcases.

@galpeter
Copy link
Contributor

@bzsolt ok, thanks for the info.

@galpeter
Copy link
Contributor

lgtm

/* 7. */
else
{
int_from_idx = - int_from_idx;
Copy link
Contributor

Choose a reason for hiding this comment

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

-int_from_idx

Copy link
Member Author

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?

Copy link
Contributor

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.

Copy link
Member Author

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.

@egavrin
Copy link
Contributor

egavrin commented Jun 21, 2015

make push after fixing

The index-dependant builtins didn't handle correctly the positive Infinity value.

JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély [email protected]
@galpeter
Copy link
Contributor

Rebased & merged: 3f28cb3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Undesired behaviour ecma builtins Related to ECMA built-in routines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants