Skip to content
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

Implement Function.prototype.bind function #321

Merged
merged 1 commit into from Jul 15, 2015
Merged

Implement Function.prototype.bind function #321

merged 1 commit into from Jul 15, 2015

Conversation

bzsolt
Copy link
Member

@bzsolt bzsolt commented Jul 7, 2015

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

@galpeter galpeter added the ecma builtins Related to ECMA built-in routines label Jul 7, 2015
@galpeter galpeter added this to the ECMA builtins milestone Jul 7, 2015
@galpeter galpeter mentioned this pull request Jul 7, 2015
Closed
11 tasks
@egavrin egavrin added the critical Raises security concerns label Jul 7, 2015
@egavrin
Copy link
Contributor

egavrin commented Jul 7, 2015

Good to me

@ruben-ayrapetyan ruben-ayrapetyan self-assigned this Jul 7, 2015
ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 7, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-function-prototype.cpp
}

/* 9. */
ecma_value_t *arg_list = static_cast <ecma_value_t *> (mem_heap_alloc_block (arg_list_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use ecma-collection instead of heap array in the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

_p

@egavrin egavrin assigned bzsolt and unassigned ruben-ayrapetyan Jul 9, 2015
@bzsolt
Copy link
Member Author

bzsolt commented Jul 10, 2015

I've updated it according the reviews.

@egavrin
Copy link
Contributor

egavrin commented Jul 10, 2015

@bzsolt please rebase to master

@bzsolt bzsolt force-pushed the function_prototype_bind branch 2 times, most recently from 9ff9a76 to 7b9b0e8 Compare July 10, 2015 11:42
@bzsolt
Copy link
Member Author

bzsolt commented Jul 10, 2015

@egavrin rebased

egavrin
egavrin reviewed Jul 10, 2015
View reviewed changes
jerry-core/ecma/operations/ecma-function-object.cpp
ecma_length_t arguments_list_len, /**< length of arguments list */
ecma_length_t *total_args_count)
{
ecma_value_t *arg_list;
Copy link
Contributor

Choose a reason for hiding this comment

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

_p

@egavrin
Copy link
Contributor

egavrin commented Jul 10, 2015

Ok to me, after fixing.

galpeter
galpeter reviewed Jul 10, 2015
View reviewed changes
jerry-core/ecma/operations/ecma-function-object.cpp
ecma_function_bind_merge_arg_lists (ecma_object_t *func_obj_p, /**< Function object */
const ecma_value_t *arguments_list_p, /**< arguments list */
ecma_length_t arguments_list_len, /**< length of arguments list */
ecma_length_t *total_args_count)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comment on total_args_count

@bzsolt
Copy link
Member Author

bzsolt commented Jul 10, 2015

The PR is updated.

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 10, 2015
View reviewed changes
jerry-core/ecma/operations/ecma-function-object.cpp
ecma_object_t *target_func_obj_p = ECMA_GET_NON_NULL_POINTER (ecma_object_t,
target_function_prop_p->u.internal_property.value);

ecma_value_t bound_this_value = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

"Empty" value would not be correct in the case. Could you, please, initialize this using ecma_make_simple_value?

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 fixed it.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, in what cases the bound_this_prop_p could be NULL?

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've checked this, it looks the bound_this_prop_p never could be NULL.
Seems it remained from a previous version. Thanks for the notice.

@ruben-ayrapetyan
Copy link
Contributor

After fixing #321 (diff), looks good to me.

egavrin
egavrin reviewed Jul 10, 2015
View reviewed changes
jerry-core/ecma/base/ecma-helpers.cpp
{
ecma_free_values_collection (ECMA_GET_NON_NULL_POINTER (ecma_collection_header_t,
property_value),
false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Broken formatting

Copy link
Member Author

Choose a reason for hiding this comment

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

This format is used in L745 and L756.
What would be the appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bzsolt ah, my bad. Missed. ()

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, it looks weird. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@bzsolt maybe we can do like that?

ecma_free_values_collection (ECMA_GET_NON_NULL_POINTER (ecma_collection_header_t, property_value),
                             false);

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 think, it looks better, I've updated this.

@egavrin
Copy link
Contributor

egavrin commented Jul 10, 2015

Still good.

@bzsolt bzsolt force-pushed the function_prototype_bind branch 3 times, most recently from 83d3628 to 63a580e Compare July 13, 2015 08:44
@zherczeg
Copy link
Member

+1 lgtm

@ruben-ayrapetyan
Copy link
Contributor

Looks good to me

@egavrin egavrin assigned bzsolt and unassigned zherczeg Jul 13, 2015
@bzsolt
Implement Function.prototype.bind function
1e90f83 
JerryScript-DCO-1.0-Signed-off-by: Zsolt Borbély zsborbely.u-szeged@partner.samsung.com
@zherczeg
Copy link
Member

make push

@kkristof kkristof merged commit 1e90f83 into jerryscript-project:master Jul 15, 2015
@bzsolt bzsolt deleted the function_prototype_bind branch August 7, 2015 14:31
@zherczeg zherczeg mentioned this pull request Feb 5, 2016
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
critical Raises security concerns ecma builtins Related to ECMA built-in routines
Projects
None yet
Milestone
ECMA builtins
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bzsolt @egavrin @ruben-ayrapetyan @zherczeg @galpeter @kkristof