Implement toString and join for TypedArrays. #2255

Achie72 · 2018-03-27T10:16:06Z

Fixed the issue that caused typedarray's toString() to output [object type], where type is the called typedarray's type, because there was no implementation for toString, and join, and default object toString was called. Now it prints out elements just as normal arrays, and as expected. Also added typedArray-tostring.js and typedArray-join.js with Asserts to test the features.

JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

LaszloLango · 2018-03-27T11:25:25Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-float32array-prototype.c

- * limitations under the License.
- */
+*
+* Licensed under the Apache License, Version 2.0 (the "License");


Please do not change the license header

It should be just a copy-paste issue, since i cannot find any difference (Compared it to ecma-builtin-boolean.c to avoid maybe same issue in another modified file).

galpeter · 2018-03-27T11:47:52Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-float32array-prototype.inc.h

+/* Routine properties:
+*  (property name, C routine name, arguments number or NON_FIXED, value of the routine's length property) */
+ROUTINE (LIT_MAGIC_STRING_TO_STRING_UL, ecma_builtin_float32array_prototype_object_to_string, 0, 0)
+ROUTINE (LIT_MAGIC_STRING_JOIN,  ecma_builtin_float32array_prototype_join, 1, 1)


These ROUTINES should not be defined per typedarray version, but simply in the typedarray-prototype file. Please see the ecma_builtin_typedarray_prototype_every implementation on how this can be done. By putting the implementation in the correct place we can reduce the code duplications.

galpeter · 2018-03-27T11:49:39Z

Additional extra note: this PR does not fix the toString method (well it does but it's akward) as there was no toString implementation before. Thus this PR implements the correct toString behaviour for the TypedArrays. Also not just the toString but the join method is also implemented here which should be mentioned in the commit message.

zherczeg

There are a lot of indentation issues with this patch, please fix all of them.

zherczeg · 2018-04-03T07:07:34Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+ */
+static ecma_value_t
+ecma_op_typedarray_get_to_string_at_index (ecma_object_t *obj_p, /**< this object */
+                                     uint32_t index) /**< array index */


add some spaces before uint32_t.

zherczeg · 2018-04-03T07:07:56Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+
+  ECMA_TRY_CATCH (index_value,
+                 ecma_op_object_get (obj_p, index_string_p),
+                 ret_value);


Also wrong indentation.

zherczeg · 2018-04-03T07:08:06Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+                 ret_value);
+
+  if (ecma_is_value_undefined (index_value)
+     || ecma_is_value_null (index_value))


zherczeg · 2018-04-03T07:08:29Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+ */
+static ecma_value_t
+ecma_builtin_typedarray_prototype_join (const ecma_value_t this_arg, /**< this argument */
+                                  const ecma_value_t separator_arg) /**< separator argument */


LaszloLango · 2018-04-05T07:50:59Z

jerry-core/lit/lit-magic-strings.inc.h

@@ -110,7 +110,8 @@ LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_EXEC, "exec")
 #if !defined (CONFIG_DISABLE_ES2015_TYPEDARRAY_BUILTIN)
 LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_FROM, "from")
 #endif
-#if !defined (CONFIG_DISABLE_ARRAY_BUILTIN)
+#if !defined (CONFIG_DISABLE_ARRAY_BUILTIN) \
+|| !defined (CONFIG_DISABLE_ES2015_TYPEDARRAY_BUILTIN)


wrong indentation

That code was generated by gen-magic-strings.py.

LaszloLango · 2018-04-05T07:51:27Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.inc.h

@@ -45,6 +45,10 @@ ACCESSOR_READ_ONLY (LIT_MAGIC_STRING_LENGTH,
                    ecma_builtin_typedarray_prototype_length_getter,
                    ECMA_PROPERTY_FIXED)

+/* Routine properties:
+*  (property name, C routine name, arguments number or NON_FIXED, value of the routine's length property) */


wrong indentation, missing space before *

LaszloLango · 2018-04-05T11:04:12Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+  ecma_value_t ret_value = ECMA_VALUE_EMPTY;
+  ecma_string_t *index_string_p = ecma_new_ecma_string_from_uint32 (index);
+
+  ECMA_TRY_CATCH (index_value,


We've started the elimination of ECMA_TRY_CATCH, so it would be better to avoid the usage of it.

I'll look up the whole file for it then, is there a doc or something about the usage of the other method?

There is no documentation, but it easy to avoid the usage of it. Check the PR of @galpeter #2246. You must check the returned ecma_value_t with ECMA_IS_VALUE_ERROR and return if it is an error. Don't forget to free everything before you return. E.g:

static ecma_value_t ecma_op_typedarray_get_to_string_at_index (ecma_object_t *obj_p, /**< this object */ uint32_t index) /**< array index */ { ecma_string_t *index_string_p = ecma_new_ecma_string_from_uint32 (index); ecma_value_t index_value = ecma_op_object_get (obj_p, index_string_p); ecma_deref_ecma_string (index_string_p); if (ECMA_IS_VALUE_ERROR (index_value)) { return index_value; } ecma_value_t ret_val = ECMA_VALUE_EMPTY; if (ecma_is_value_undefined (index_value) || ecma_is_value_null (index_value)) { ret_val = ecma_make_magic_string_value (LIT_MAGIC_STRING__EMPTY); } else { ret_val = ecma_op_to_string (index_value); } ecma_free_value (index_value); return ret_val; } /* ecma_op_typedarray_get_to_string_at_index */

rerobika · 2018-04-09T18:11:08Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+    ecma_ref_ecma_string (return_string_p);
+
+    /* 9-10. */
+    for (uint32_t k = 1; ecma_is_value_empty (ret_value) && (k < length); k++)


This loop does not change ret_value thereby the function call ecma_is_value_empty (ret_value) in every iteration is unnecessary. This loop can be merged with the if statement below.

galpeter · 2018-04-10T09:26:46Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+  {
+    return join_value;
+  }
+  if (!ecma_op_is_callable (join_value))


please add a newline before the if

galpeter · 2018-04-10T09:27:21Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+
+  /* 4-5. */
+  ecma_value_t separator_value = ecma_op_typedarray_get_separator_string (separator_arg);
+  if( ECMA_IS_VALUE_ERROR (separator_value))


the space should be before the (

galpeter · 2018-04-10T09:27:52Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+    return separator_value;
+  }
+  if (length == 0)
+


the empty line should be before the if.

galpeter · 2018-04-10T09:29:23Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+  }
+
+  if (ecma_is_value_undefined (index_value)
+     || ecma_is_value_null (index_value))


a space is needed before the ||.

zherczeg · 2018-04-12T09:46:05Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+
+  if (ECMA_IS_VALUE_ERROR (index_value))
+  {
+    return index_value;


index_string_p must be freed.

zherczeg · 2018-04-12T09:47:04Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+} /* ecma_builtin_typedarray_prototype_join */
+
+
+/**


One newline above.

LaszloLango · 2018-04-13T09:22:03Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+
+  /* 3. */
+  uint32_t length = ecma_number_to_uint32 (length_number);
+


length_number won't be freed in any cases. ECMA_OP_TO_NUMBER_FINALIZE (length_number); should be moved up. e.g.:

uint32_t length = 0; ECMA_OP_TO_NUMBER_TRY_CATCH (length_number, length_value, ret_value); /* 3. */ length = ecma_number_to_uint32 (length_number); ECMA_OP_TO_NUMBER_FINALIZE (length_number); if (ECMA_IS_VALUE_ERROR (ret_value) { ...

LaszloLango · 2018-04-13T09:25:01Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+  ecma_value_t ret_value = ECMA_VALUE_EMPTY;
+  ecma_string_t *index_string_p = ecma_new_ecma_string_from_uint32 (index);
+  ecma_value_t index_value = ecma_op_object_get (obj_p, index_string_p);
+


index_string_p can be freed here, so you can replace the two ecma_deref_ecma_string (index_string_p); calls by one line.

JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

zherczeg · 2018-04-17T10:18:13Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+ *         Returned value must be freed with ecma_free_value.
+ */
+static ecma_value_t
+ecma_op_typedarray_get_to_string_at_index (ecma_object_t *obj_p, /**< this object */


Why don't we return with ecma_string_t*? It seems concatenation uses it anyway.

@zherczeg there is a return case where the index_value "errored" and that is probably not a ecma_string_t*

galpeter

lgtm.

galpeter · 2018-04-19T07:10:22Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+ *         Returned value must be freed with ecma_free_value.
+ */
+static ecma_value_t
+ecma_op_typedarray_get_to_string_at_index (ecma_object_t *obj_p, /**< this object */


@zherczeg there is a return case where the index_value "errored" and that is probably not a ecma_string_t*

zherczeg

LGTM

zherczeg · 2018-04-19T07:59:35Z

jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c

+ *         Returned value must be freed with ecma_free_value.
+ */
+static ecma_value_t
+ecma_op_typedarray_get_to_string_at_index (ecma_object_t *obj_p, /**< this object */


LaszloLango

LGTM

JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

LaszloLango requested changes Mar 27, 2018

View reviewed changes

galpeter reviewed Mar 27, 2018

View reviewed changes

Achie72 force-pushed the toString branch from b0994ea to 518b927 Compare March 28, 2018 18:14

Achie72 changed the title ~~Fix TypedArray toString method for all types.~~ Implement toString and join for TypedArrays. Mar 28, 2018

Achie72 force-pushed the toString branch 2 times, most recently from a13243c to 2a15bf7 Compare March 29, 2018 16:19

zherczeg requested changes Apr 3, 2018

View reviewed changes

Achie72 force-pushed the toString branch 2 times, most recently from 93ae976 to 5e0ddfa Compare April 4, 2018 08:43

LaszloLango requested changes Apr 5, 2018

View reviewed changes

LaszloLango reviewed Apr 5, 2018

View reviewed changes

Achie72 force-pushed the toString branch from 5e0ddfa to 8bb39b3 Compare April 9, 2018 11:15

rerobika reviewed Apr 9, 2018

View reviewed changes

galpeter reviewed Apr 10, 2018

View reviewed changes

Achie72 force-pushed the toString branch 2 times, most recently from b5a7ee7 to ec73aab Compare April 10, 2018 11:32

zherczeg requested changes Apr 12, 2018

View reviewed changes

Achie72 force-pushed the toString branch 2 times, most recently from b6eebe5 to 7950d02 Compare April 13, 2018 08:16

LaszloLango requested changes Apr 13, 2018

View reviewed changes

Achie72 force-pushed the toString branch from 7950d02 to 1a46548 Compare April 13, 2018 09:53

Implement toString and join for TypedArrays.

1ef91a6

JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

Achie72 force-pushed the toString branch from 1a46548 to 1ef91a6 Compare April 16, 2018 07:24

zherczeg reviewed Apr 17, 2018

View reviewed changes

galpeter approved these changes Apr 19, 2018

View reviewed changes

zherczeg approved these changes Apr 19, 2018

View reviewed changes

LaszloLango approved these changes Apr 19, 2018

View reviewed changes

LaszloLango merged commit 3df6ef3 into jerryscript-project:master Apr 19, 2018

jimmy-huang pushed a commit to jimmy-huang/jerryscript that referenced this pull request May 8, 2018

Implement toString and join for TypedArrays. (jerryscript-project#2255)

e72d03c

JerryScript-DCO-1.0-Signed-off-by: Bela Toth [email protected]

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed


		/* 3. */
		uint32_t length = ecma_number_to_uint32 (length_number);

		} /* ecma_builtin_typedarray_prototype_join */


		/**

Implement toString and join for TypedArrays. #2255

Implement toString and join for TypedArrays. #2255

Uh oh!

Conversation

Achie72 commented Mar 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpeter commented Mar 27, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galpeter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Achie72 commented Mar 27, 2018 •

edited

Loading

LaszloLango Apr 6, 2018 •

edited

Loading