Fix the indexing of Array builtin functions. by bzsolt · Pull Request #196 · jerryscript-project/jerryscript · GitHub
Skip to content

Fix the indexing of Array builtin functions. - #196

Closed
bzsolt wants to merge 1 commit into
jerryscript-project:masterfrom
bzsolt:fix_array_builtin_indexing
Closed

Fix the indexing of Array builtin functions.#196
bzsolt wants to merge 1 commit into
jerryscript-project:masterfrom
bzsolt:fix_array_builtin_indexing

Conversation

@bzsolt

@bzsolt bzsolt commented Jun 16, 2015

Copy link
Copy Markdown
Member

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

JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély zsborbely.u-szeged@partner.samsung.com

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jun 16, 2015
View reviewed changes
Comment thread jerry-core/ecma/base/ecma-helpers-number.cpp Outdated

@ruben-ayrapetyan ruben-ayrapetyan Jun 16, 2015

Copy link
Copy Markdown

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?

@bzsolt bzsolt Jun 16, 2015

Copy link
Copy Markdown
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.

@ruben-ayrapetyan ruben-ayrapetyan Jun 16, 2015

Copy link
Copy Markdown

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
egavrin
egavrin reviewed Jun 16, 2015
View reviewed changes
Comment thread jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp Outdated

@egavrin egavrin Jun 16, 2015

Copy link
Copy Markdown
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.

@bzsolt bzsolt Jun 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed.

@galpeter

galpeter commented Jun 17, 2015

Copy link
Copy Markdown
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

bzsolt commented Jun 17, 2015

Copy link
Copy Markdown
Member Author

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

galpeter commented Jun 17, 2015

Copy link
Copy Markdown
Contributor

@bzsolt ok, thanks for the info.

@galpeter

galpeter commented Jun 17, 2015

Copy link
Copy Markdown
Contributor

lgtm

egavrin
egavrin reviewed Jun 21, 2015
View reviewed changes
Comment thread jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp Outdated

@egavrin egavrin Jun 21, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

-int_from_idx

@bzsolt bzsolt Jun 22, 2015

Copy link
Copy Markdown
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?

@egavrin egavrin Jun 22, 2015

Copy link
Copy Markdown
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.

@bzsolt bzsolt Jun 23, 2015

Copy link
Copy Markdown
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

egavrin commented Jun 21, 2015

Copy link
Copy Markdown
Contributor

make push after fixing

Zsolt Borbély
Fix the indexing of Array builtin functions.
309ce50 
The index-dependant builtins didn't handle correctly the positive Infinity value.

JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély zsborbely.u-szeged@partner.samsung.com
@galpeter

galpeter commented Jun 23, 2015

Copy link
Copy Markdown
Contributor

@galpeter galpeter closed this Jun 23, 2015
@bzsolt
bzsolt deleted the fix_array_builtin_indexing branch June 24, 2015 07:36
@zherczeg zherczeg mentioned this pull request Feb 5, 2016
Closed
This was referenced May 17, 2020
Closed
Closed
This was referenced Oct 20, 2020
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@bzsolt bzsolt

Labels

bug Undesired behaviour ecma builtins Related to ECMA built-in routines

Projects

None yet

Milestone

ECMA builtins

Development

Successfully merging this pull request may close these issues.

4 participants

@bzsolt @galpeter @egavrin @ruben-ayrapetyan