Fix assert in Object.defineProperties function #137
Conversation
|
Please reference the related issue somewhere. |
|
Fix for issue #131 |
| @@ -333,7 +333,7 @@ ecma_builtin_object_object_define_properties (ecma_value_t this_arg __attr_unuse | |||
| { | |||
| ecma_completion_value_t ret_value = ecma_make_empty_completion_value (); | |||
|
|
|||
| if (!ecma_is_value_object (arg1)) | |||
| if (!ecma_is_value_object (arg1) || !ecma_is_value_object (arg2)) | |||
There was a problem hiding this comment.
Could you, please, update implementation to follow ECMA-defined pseudocode?
In the ECMA description of the procedure there is no type check, there is ToObject conversion.
There was a problem hiding this comment.
Could you also, please, add comments with step numbers according to ECMA pseudocode, like // 5.c?
There was a problem hiding this comment.
as part of the refactoring I'm going to do this as well
ruben-ayrapetyan
added
bug
|
I'm refactoring this function because we've found some corner cases where the actual result differs from the expected. |
|
Hi, I have updated the patch. |
|
|
||
| if (!ecma_is_completion_value_throw (ret_value)) | ||
| { | ||
| property_descriptor_number++; |
There was a problem hiding this comment.
The line could be moved to line 471, and the if could be removed then.
|
Hi, I have updated the patch. |
|
I've added your test case to the test. |
|
@kkristof, great. What about adding test for ' |
|
This isn't that case? |
No.
In the comment, I didn't asked you to add additional test case, but to update the tests cases that you have already added. So,
Currently, we have tests for first and third case, but not for the second. |
|
Hi, I have updated the test. |
|
Looks good to me. |
| uint32_t index = 0; | ||
| for (property_p = ecma_get_property_list (props_p); | ||
| property_p != NULL; | ||
| property_p = ECMA_GET_POINTER (ecma_property_t, property_p->next_property_p), index++) |
There was a problem hiding this comment.
Do we need the index here at all?
There was a problem hiding this comment.
No, we don't. thank you :)
|
Hi, I have updated the branch, but I haven't renamed the |
|
looks good to me. @ruben-ayrapetyan what do you think? |
|
|
||
| uint32_t index = 0; | ||
| for (property_p = ecma_get_property_list (props_p); | ||
| property_p != NULL && ecma_is_completion_value_empty (ret_value); |
There was a problem hiding this comment.
The && ecma_is_completion_value_empty (ret_value) condition part seems not to be necessary, as ret_value is not changed in the loop.
After removing the unnecessary condition part (#137 (diff)), the changes would be ok. |
Fix assert and corner cases in Object.defineProperties function. JerryScript-DCO-1.0-Signed-off-by: Kristof Kosztyo [email protected]
|
Hi, I have updated the patch. |
|
Looks good to me |
|
|
JerryScript-DCO-1.0-Signed-off-by: Kristof Kosztyo [email protected]