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.

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

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

kisbg
Copy link

@kisbg kisbg commented Dec 16, 2020

JerryScript-DCO-1.0-Signed-off-by: bence gabor kis [email protected]

Comment on lines 589 to 592
/**
* create class object operation.
*
*
* @return ecma value
* Returned value must be freed with ecma_free_value
*/
Copy link
Member

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 */
Copy link
Member

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;
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@rerobika rerobika added the ES.next Related to ES2015+ features label Dec 16, 2020
@kisbg kisbg force-pushed the newtarget-boolean branch from ba97957 to 5eedb40 Compare December 16, 2020 09:51
/**
* Create a ECMA_OBJECT_TYPE_CLASS object from the given arguments.
*
*
Copy link
Member

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);

Copy link
Member

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);

Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@kisbg kisbg force-pushed the newtarget-boolean branch from 5eedb40 to 7c8646d Compare December 16, 2020 11:58
Copy link
Member

@rerobika rerobika left a 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))
Copy link
Member

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"
Copy link
Member

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 */
Copy link
Member

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.

Copy link
Author

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>
Copy link
Member

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?

Copy link
Author

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]
@kisbg kisbg force-pushed the newtarget-boolean branch from 7c8646d to 5a63e0f Compare December 17, 2020 07:55
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@zherczeg zherczeg merged commit 1937f82 into jerryscript-project:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ES.next Related to ES2015+ features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants