Skip to content

Rework function call. #2414

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

Merged
merged 1 commit into from
Jul 11, 2018

Conversation

zherczeg
Copy link
Member

Furthermore add a construct flag, which disallows calling certain functions without new. Constructing bound arrow functions correctly throws error now.

It seems the change has close to no effect:
Binary: 50 byte reduction, perf: -0.004%, mem: 0.000%

@LaszloLango
Copy link
Contributor

Please add a test for this.

@zherczeg zherczeg force-pushed the construct_improvement branch from f80dd6a to f4b2bea Compare June 29, 2018 09:20
@zherczeg
Copy link
Member Author

Test case added.

@@ -639,27 +682,28 @@ ecma_op_function_call (ecma_object_t *func_obj_p, /**< Function object */
* Returned value must be freed with ecma_free_value
*/
static ecma_value_t
ecma_op_function_construct_simple_or_external (ecma_object_t *func_obj_p, /**< Function object */
ecma_op_function_construct_ecma_or_external (ecma_object_t *func_obj_p, /**< Function object */
const ecma_value_t *arguments_list_p, /**< arguments list */
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong indentation

{
if (JERRY_UNLIKELY (ecma_get_object_is_builtin (func_obj_p)))
{
return ecma_builtin_dispatch_call (func_obj_p,
Copy link
Member

Choose a reason for hiding this comment

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

Built-in function objects should never have a construct_flag, but it would be nice to see a JERRY_ASSERT (!ecma_op_function_has_construct_flag (arguments_list_p)) before returning with ecma_builtin_dispatch_call.

This case currently seems impossible but the assertion could prevent the wrong usage of the ecma_op_function_set_construct_flag in the future.

Furthermore add a construct flag, which disallows calling certain
functions without new. Constructing bound arrow functions correctly
throws error now.

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
@zherczeg zherczeg force-pushed the construct_improvement branch from f4b2bea to a59cf4f Compare July 10, 2018 13:11
@zherczeg
Copy link
Member Author

Thank you for the comments, patch is updated.

Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

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

LGTM

@rerobika
Copy link
Member

LGTM (informally)

Copy link
Contributor

@yichoi yichoi left a comment

Choose a reason for hiding this comment

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

LGTM

@yichoi yichoi merged commit 04dcefe into jerryscript-project:master Jul 11, 2018
@zherczeg zherczeg deleted the construct_improvement branch July 13, 2018 07:12
yorkie added a commit to yodaos-project/ShadowNode that referenced this pull request Oct 20, 2018
Furthermore add a construct flag, which disallows calling certain
functions without new. Constructing bound arrow functions correctly
throws error now.

Author: Zoltan Herczeg<[email protected]>
PR-URL: jerryscript-project/jerryscript#2414
yorkie added a commit to yodaos-project/ShadowNode that referenced this pull request Oct 23, 2018
Furthermore add a construct flag, which disallows calling certain
functions without new. Constructing bound arrow functions correctly
throws error now.

Author: Zoltan Herczeg<[email protected]>
PR-URL: jerryscript-project/jerryscript#2414
yorkie added a commit to yodaos-project/ShadowNode that referenced this pull request Oct 24, 2018
Furthermore add a construct flag, which disallows calling certain
functions without new. Constructing bound arrow functions correctly
throws error now.

Author: Zoltan Herczeg<[email protected]>
PR-URL: jerryscript-project/jerryscript#2414
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants