Skip to content

Implemented Array.prototype.sort() #95

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Implemented Array.prototype.sort() #95

wants to merge 1 commit into from

Conversation

dbatyai
Copy link
Member

@dbatyai dbatyai commented May 22, 2015

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai [email protected]
JerryScript-DCO-1.0-Signed-off-by: Szilard Ledan [email protected]

@ILyoan ILyoan mentioned this pull request May 22, 2015
Closed
25 tasks
@dbatyai dbatyai added the ecma builtins Related to ECMA built-in routines label May 22, 2015
@galpeter galpeter added this to the ECMA builtins milestone May 22, 2015
@dbatyai dbatyai force-pushed the array_prototype_sort branch 2 times, most recently from f939751 to 233dd6e Compare May 26, 2015 11:42
egavrin
egavrin reviewed May 27, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we're using 2 here?

@egavrin egavrin self-assigned this May 27, 2015
@dbatyai dbatyai force-pushed the array_prototype_sort branch from 233dd6e to 0fd2a85 Compare May 28, 2015 14:32
@dbatyai
Copy link
Member Author

dbatyai commented May 28, 2015

Hi @egavrin,
I've made some updates. The previous implementation, even though it was working as expected, had some problems with indexing. I've gone over it and fixed the indexes, so now it works as it should.
I've added some comments as well to make it easier to read.

ruben-ayrapetyan
ruben-ayrapetyan reviewed May 28, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

@dbatyai, could you, please, add assertions with bound checking in the function?

@dbatyai dbatyai force-pushed the array_prototype_sort branch from 0fd2a85 to 761c079 Compare May 28, 2015 15:26
@dbatyai
Copy link
Member Author

dbatyai commented May 28, 2015

@ruben-ayrapetyan, I've added asserts to make sure we stay in bounds.

@ruben-ayrapetyan
Copy link
Contributor

@dbatyai, thank you!

@egavrin
Copy link
Contributor

egavrin commented May 29, 2015

May we push it?

ruben-ayrapetyan
ruben-ayrapetyan reviewed May 29, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add _p suffix.

@dbatyai dbatyai force-pushed the array_prototype_sort branch from 761c079 to 7fe0d39 Compare May 29, 2015 11:40
@dbatyai
Copy link
Member Author

dbatyai commented May 29, 2015

I've updated the patch.

@ruben-ayrapetyan
Copy link
Contributor

Looks good to me.

ruben-ayrapetyan
ruben-ayrapetyan reviewed May 29, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-array-prototype.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add FIXME to the comment.
Seems that we should change the ecma_op_create_array_object interface to take another type for length argument.

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, raise an issue about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added FIXME, and opened an issue about it: #133

@dbatyai dbatyai force-pushed the array_prototype_sort branch from 7fe0d39 to 9a88ef0 Compare May 29, 2015 12:31
@egavrin
Copy link
Contributor

egavrin commented May 29, 2015

@dbatyai great! make push

@szledan @dbatyai
Implemented Array.prototype.sort()
0fc82e2 
JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai [email protected]
JerryScript-DCO-1.0-Signed-off-by: Szilard Ledan [email protected]
@dbatyai dbatyai force-pushed the array_prototype_sort branch from 9a88ef0 to 0fc82e2 Compare May 29, 2015 13:16
@dbatyai
Copy link
Member Author

dbatyai commented May 29, 2015

merged: 6b8e34a

@dbatyai dbatyai closed this May 29, 2015
@egavrin egavrin deleted the array_prototype_sort branch May 29, 2015 19:48
@zherczeg zherczeg mentioned this pull request Jul 20, 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

No one assigned

Labels

ecma builtins Related to ECMA built-in routines

Projects

None yet

Milestone

ECMA builtins

Development

Successfully merging this pull request may close these issues.

5 participants

@dbatyai @ruben-ayrapetyan @egavrin @galpeter @szledan