-
Notifications
You must be signed in to change notification settings - Fork 677
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
Added new target support to Boolean, String, Number Object #4368
Conversation
/** | ||
* create class object operation. | ||
* | ||
* | ||
* @return ecma value | ||
* Returned value must be freed with ecma_free_value | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* Create a ECMA_OBJECT_TYPE_CLASS object from the given arguments.
*
* @return ecma_value - constructed object
*/
ecma_value_t | ||
ecma_op_create_class_object (ecma_builtin_id_t proto_id, /**< prototype id */ | ||
ecma_value_t value, /**< ecma value */ | ||
uint16_t lit_id) /**< magic string id */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lit_id
->class_id
@@ -43,19 +45,30 @@ ecma_op_create_number_object (ecma_value_t arg) /**< argument passed to the Numb | |||
{ | |||
ecma_number_t num; | |||
ecma_value_t conv_to_num_completion = ecma_op_to_number (arg, &num); | |||
|
|||
ecma_builtin_id_t proto_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lower these declarations closer to these first usage.
@@ -47,6 +49,8 @@ ecma_op_create_string_object (const ecma_value_t *arguments_list_p, /**< list of | |||
|| arguments_list_p != NULL); | |||
|
|||
ecma_value_t prim_value = ecma_make_magic_string_value (LIT_MAGIC_STRING__EMPTY); | |||
ecma_builtin_id_t proto_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
ba97957
to
5eedb40
Compare
/** | ||
* Create a ECMA_OBJECT_TYPE_CLASS object from the given arguments. | ||
* | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one new comment line.
@@ -43,19 +45,29 @@ ecma_op_create_number_object (ecma_value_t arg) /**< argument passed to the Numb | |||
{ | |||
ecma_number_t num; | |||
ecma_value_t conv_to_num_completion = ecma_op_to_number (arg, &num); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not remove this line.
@@ -47,7 +49,6 @@ ecma_op_create_string_object (const ecma_value_t *arguments_list_p, /**< list of | |||
|| arguments_list_p != NULL); | |||
|
|||
ecma_value_t prim_value = ecma_make_magic_string_value (LIT_MAGIC_STRING__EMPTY); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
5eedb40
to
7c8646d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ecma_object_t *prototype_obj_p = ecma_builtin_get (proto_id); | ||
|
||
#if ENABLED (JERRY_ESNEXT) | ||
if (JERRY_CONTEXT (current_new_target)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would copy this into a local variable.
@@ -35,6 +35,7 @@ | |||
#include "ecma-string-object.h" | |||
#include "ecma-symbol-object.h" | |||
#include "jrt-libc-includes.h" | |||
#include "jcontext.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
@@ -42,13 +44,27 @@ ecma_value_t | |||
ecma_op_create_boolean_object (ecma_value_t arg) /**< argument passed to the Boolean constructor */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An important question: is this only used by constructors? Because if it is used elsewhere, it must not use the new target. This is also true for all other functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, only used ecma_builtin_boolean_dispatch_construct
.
@@ -9668,7 +9666,6 @@ | |||
<test id="built-ins/RegExp/prototype/sticky/cross-realm.js"><reason></reason></test> | |||
<test id="built-ins/RegExp/prototype/unicode/cross-realm.js"><reason></reason></test> | |||
<test id="built-ins/SharedArrayBuffer/proto-from-ctor-realm.js"><reason></reason></test> | |||
<test id="built-ins/String/proto-from-ctor-realm.js"><reason></reason></test> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only realm tests are fixed? No normal tests for these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, there is no new target test for boolean, string and number object.
JerryScript-DCO-1.0-Signed-off-by: bence gabor kis [email protected]
7c8646d
to
5a63e0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
JerryScript-DCO-1.0-Signed-off-by: bence gabor kis [email protected]