Implemented Array.prototype.push() #7
Conversation
|
|
||
| ecma_object_t *obj_p = ecma_get_object_from_completion_value (completion_obj_this); | ||
|
|
||
| ecma_free_completion_value (completion_obj_this); |
There was a problem hiding this comment.
Hello Ilyong,
The call to ecma_free_completion_value would invalidate reference in obj_p.
So, it should be placed after obj_p becomes unnecessary.
Actually, for this simple case ECMA_TRY_CATCH / ECMA_FINALIZE macro are very useful.
With them it is possible to indicate scope of reference visilibity, handling exceptions at the same time.
ecma_completion_value_t ret_value;
ECMA_TRY_CATCH (obj_this_value, ecma_op_to_object (this_arg), ret_value);
// obj_this_value is accessible until corresponding ECMA_FINALIZE
ecma_object_t *obj_p = ecma_get_object_from_value (obj_this_value);
// ...
ECMA_FINALIZE (obj_this_value);
return ret_value;
|
Hi ruben. Thank you for the review. We, here in Korea, are in vacation till 5th may for children's day. I'll check and manage this issue regarding to your opinion as soon as back to office. |
|
Hi Ruben. I've amended PR following your guide. |
| const ecma_value_t *argument_list_p, /**< arguments list */ | ||
| ecma_length_t arguments_number) /**< number of arguments */ | ||
| { | ||
| JERRY_ASSERT (&argument_list_p != NULL && arguments_number > 0); |
There was a problem hiding this comment.
Hi Ilyong.
Couldn't the procedure receive arguments_list_p == NULL && arguments_number == 0?
Also, "&arguments_list_p" is always non-NULL.
There was a problem hiding this comment.
Wouldn't this assert if the js developer calls the method with zero arguments?
example:
var a = [];
a.push();
There was a problem hiding this comment.
It would assert. It is exactly the case, mentioned in the comment above.
|
Hi Ruben. I've changed this PR once more. Please give review again. |
| ret_value = ecma_make_normal_completion_value (num_length_value); | ||
|
|
||
| ECMA_FINALIZE (op_ret_value); | ||
| } |
There was a problem hiding this comment.
There is a point that I didn't notice during previous review.
If the [[Put]] threw an exception, and, so, ret_value = ecma_make_normal_completion_value (num_length_value); was not executed, num_length should be deallocated after the try-finalize block for [[Put]].
Could you, please, add call to ecma_dealloc_number?
Could you also, please, add "_p" suffix to num_length, like for other pointer variables?
if (ecma_is_completion_value_empty (ret_value))
{
ecma_number_t *num_length_p = ecma_alloc_number ();
*num_length_p = ecma_uint32_to_number (n);
ecma_completion_value_t completion = ecma_op_object_put (obj_p,
length_str_p,
ecma_make_number_value (num_length_p),
true);
if (unlikely (ecma_is_completion_value_throw (completion)
|| ecma_is_completion_value_exit (completion)))
{
ret_value = completion;
ecma_dealloc_number (num_length_p);
}
else
{
JERRY_ASSERT (ecma_is_completion_value_normal (completion));
ecma_free_completion_value (completion);
ret_value = ecma_make_normal_completion_value (ecma_make_number_value (num_length_p));
}
}
After adding deallocation, the changes would be ok to merge.
There was a problem hiding this comment.
OK. you're right.
You have shape eyes.
I'll fix it right now and renew the PR.
JerryScript-DCO-1.0-Signed-off-by: Ilyong Cho ily.cho@samsung.com
|
@ruben-ayrapetyan |
|
Hi Ilyong. Thank you. Changes were merged successfully. |
|
@ruben-ayrapetyan By the way, I have a question. |
|
@ILyoan |
|
@ruben-ayrapetyan |
Add support for try statement in parse_statement_.
Added new test cases to array-prototype-concat.js to improve branch coverage.
Branch coverage:
before: 10/14
after: 13/14
JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi repasics@inf.u-szeged.hu
for #3 and jerryscript-project/iotjs#9
JerryScript-DCO-1.0-Signed-off-by: Ilyong Cho ily.cho@samsung.com