[ES2015][TypedArray] Add other 8 types #1532

jiangzidong · 2017-01-18T04:37:03Z

add Uint8Array, Int16Array Uint16Array, Int32Array, Uint32Array, Float32Array Float64Array Uint8ClampedArray

Support the conversion between any pairs of those types.

JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

jiangzidong · 2017-01-18T08:18:50Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers.c

@@ -83,7 +83,7 @@ ecma_builtin_helper_object_to_string (const ecma_value_t this_arg) /**< this arg
  /* Building string "[object #type#]" where type is 'Undefined',
     'Null' or one of possible object's classes.
     The string with null character is maximum 19 characters long. */
-  const lit_utf8_size_t buffer_size = 19;
+  const lit_utf8_size_t buffer_size = 27;


because now the longest class name is Uint8ClampedArray, and 19 is not enough

Please also update the comment.

LaszloLango

Looks good in general. More tests would be great (push, pop, manipulating array elements, etc.)

LaszloLango · 2017-01-18T08:27:08Z

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

+
+  return val;
+} /* ecma_builtin_uint32array_dispatch_construct */
+


Missing end of doxygen groups comment:

/** * @} * @} * @} */

LaszloLango · 2017-01-18T08:27:59Z

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

+
+  return val;
+} /* ecma_builtin_uint8array_dispatch_construct */
+


LaszloLango · 2017-01-18T08:28:16Z

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

+
+  return val;
+} /* ecma_builtin_uint8clampedarray_dispatch_construct */
+


LaszloLango · 2017-01-18T08:29:49Z

tests/jerry-test-suite/es2015/22/22.02/22.02.01/22.02.01-011.js

+assert(a[0] === 2);
+assert(a[1] === 2);
+assert(a[2] === 0);
+assert(a[3] === 255);


Missing new line at the end of file.

jiangzidong · 2017-01-18T13:28:50Z

@LaszloLango Thanks for the comments, About the test, this patch mainly focus on the conversion of different types, and we didn't implement the method such as "forEach, fill, indexOf, ..." yet. I will add more tests about the conversion, and will implement other methods in the following patch.

jiangzidong · 2017-01-19T02:28:05Z

updated

zherczeg

Great patch!

zherczeg · 2017-01-19T11:21:15Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers.c

-     The string with null character is maximum 19 characters long. */
-  const lit_utf8_size_t buffer_size = 19;
+     The string with null character is maximum 27 characters long. */
+  const lit_utf8_size_t buffer_size = 27;
  JMEM_DEFINE_LOCAL_ARRAY (str_buffer, buffer_size, lit_utf8_byte_t);


Perhaps we should change this to:

lit_utf8_byte_t str_buffer[buffer_size];

I don't think malloc is needed here.

zherczeg · 2017-01-19T11:46:45Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+        }
+        case LIT_MAGIC_STRING_FLOAT64_ARRAY_UL:
+        {
+          *((double *) dst_buf) = (double) tmp;


Actually double support is not required for jerry. Perhaps we should make it optional.

I see,
what about the following rule?
"only if we define both typedarray_builtin and CONFIG_ECMA_NUMBER_FLOAT64, we will implement Float64Array"

zherczeg · 2017-01-19T11:48:31Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+      {
+        case LIT_MAGIC_STRING_INT8_ARRAY_UL:
+        {
+          GET_VALUE_FROM_ARRAYBUFFER (int8_t);


For me these macros are hard to read, since they return something but no return value.

tmp = GET_VALUE_FROM_ARRAYBUFFER (int8_t, src_buf);

This would look better.

It seems like the function-like macro have return value. And because we have var definition inside the macro, we should use gcc' statement exprs instead of c99's comma exprs. It may damage the portability.

so I choose to use GET_VALUE_FROM_ARRAYBUFFER (int8_t, src_buf, tmp);

zherczeg · 2017-01-19T11:57:26Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+          }
+          else
+          {
+            tmp_int = (int64_t) tmp;


Do we need int64 casting here? Not exactly a cheap operation on 32 bit systems.

In this case, yes, we should remove that because in the 'else' block, the tmp's range is 0 - 255.

zherczeg · 2017-01-19T11:58:27Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+      }
+      case LIT_MAGIC_STRING_FLOAT64_ARRAY_UL:
+      {
+        *((double *) target_p) = (double) value_num;


This seems a lot of code duplication, perhaps we should combine them. Code size matters :)

zherczeg · 2017-01-20T08:48:26Z

jerry-core/ecma/operations/ecma-typedarray-object.c:304: style(variableScope): The scope of the variable 'tmp' can be reduced.
This is the fail

LaszloLango · 2017-01-23T12:52:47Z

@jiangzidong please fix the doxygen related errors. Run doxygen and check the related warnings. There are some issues with your group names.

* \addtogroup float32array prototype ECMA Float32Array.prototype object built-in
must be
* \addtogroup float32arrayprototype ECMA Float32Array.prototype object built-in
and * \addtogroup string ECMA Float32Array object built-in
must be
* \addtogroup float32array ECMA Float32Array object built-in
etc.

jiangzidong · 2017-01-23T13:20:07Z

@LaszloLango updated. sorry that i didn't notice the doxygen part.

zherczeg

Apart from some style issues, the patch is good for me. Please fix them before landing.

zherczeg · 2017-01-23T14:12:54Z

jerry-core/ecma/builtin-objects/ecma-builtins.inc.h

+         true,
+         true,
+         float32array)
+#if CONFIG_ECMA_NUMBER_TYPE == CONFIG_ECMA_NUMBER_FLOAT64


Put a newline before this # if

zherczeg · 2017-01-23T14:19:05Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+                        lit_magic_string_id_t class_id) /**< class name of the typedarray */
+{
+
+#define SET_ELEMENT(type, location, number) \


No nned a newline above this.

zherczeg · 2017-01-23T14:20:42Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+} /* set_typedarray_element */
+
+
+


One newline is enough.

zherczeg · 2017-01-23T14:21:24Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+    for (uint32_t i = 0; i < array_length; i++)
+    {
+      ecma_number_t tmp = get_typedarray_element (src_buf,
+                                    ecma_object_get_class_name (typedarray_p));


Line alignment.

LaszloLango

LGTM, after minor changes

LaszloLango · 2017-01-24T08:41:21Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+get_typedarray_element (lit_utf8_byte_t *src, /**< the location in the internal arraybuffer */
+                        lit_magic_string_id_t class_id) /**< class name of the typedarray */
+{
+#define GET_ELEMENT(type, location, number) \


Place of GET_ELEMENT and SET_ELEMENT macros are a bit odd to me. I'd move them out of the functions add descriptions to them too.

LaszloLango · 2017-01-24T08:45:09Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+    }
+    case LIT_MAGIC_STRING_FLOAT32_ARRAY_UL:
+    {
+      *((float *) dst) = (float) value;


SET_ELEMENT (float, dst, value);

for float/double type, it cannot use the SET_ELEMENT macro.
SET_ELEMRNT macro will convert the ecma_number_t into a integer type, then convert to the target type. float -> int -> float will lose precision.

Why I have to convert to integer first, because "behavior of float type convert to shorter int type or unsigned int type is undefined by C99"

LaszloLango · 2017-01-24T08:45:29Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+#if CONFIG_ECMA_NUMBER_TYPE == CONFIG_ECMA_NUMBER_FLOAT64
+    case LIT_MAGIC_STRING_FLOAT64_ARRAY_UL:
+    {
+      *((double *) dst) = (double) value;


SET_ELEMENT (double, dst, value);

add Uint8Array, Int16Array Uint16Array, ... JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

LaszloLango · 2017-01-24T09:43:58Z

LGTM

jiangzidong force-pushed the typedarray0118 branch from 9875727 to a120c9e Compare January 18, 2017 08:00

LaszloLango added ecma builtins Related to ECMA built-in routines feature request Requested feature labels Jan 18, 2017

jiangzidong commented Jan 18, 2017

View reviewed changes

LaszloLango requested changes Jan 18, 2017

View reviewed changes

rtakacs mentioned this pull request Jan 18, 2017

JerryScript update jerryscript-project/iotjs#602

Closed

jiangzidong force-pushed the typedarray0118 branch from a120c9e to c9c815f Compare January 19, 2017 02:24

jiangzidong force-pushed the typedarray0118 branch from c9c815f to a317755 Compare January 19, 2017 02:35

zherczeg requested changes Jan 19, 2017

View reviewed changes

jiangzidong force-pushed the typedarray0118 branch from a317755 to f98a846 Compare January 20, 2017 01:40

jiangzidong force-pushed the typedarray0118 branch from f98a846 to b9b9a6a Compare January 20, 2017 12:24

jiangzidong force-pushed the typedarray0118 branch from b9b9a6a to 0715019 Compare January 23, 2017 13:18

zherczeg approved these changes Jan 23, 2017

View reviewed changes

jiangzidong force-pushed the typedarray0118 branch from 0715019 to b3b870f Compare January 24, 2017 00:39

tilmannOSG changed the title ~~[ES2015][TypedArray] add other 8 types~~ [ES2015][TypedArray] Add other 8 types Jan 24, 2017

jiangzidong mentioned this pull request Jan 24, 2017

Add doxygen checker #1540

Merged

LaszloLango requested changes Jan 24, 2017

View reviewed changes

jiangzidong force-pushed the typedarray0118 branch from b3b870f to cb020a5 Compare January 24, 2017 09:11

[ES2015][TypedArray] add other 8 types

cb020a5

add Uint8Array, Int16Array Uint16Array, ... JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

LaszloLango approved these changes Jan 24, 2017

View reviewed changes

LaszloLango merged commit 0547b31 into jerryscript-project:master Jan 24, 2017

jiangzidong deleted the typedarray0118 branch February 8, 2017 02:43

zherczeg mentioned this pull request Mar 27, 2018

Nominating Robert Sipka (robertsipka) to become a reviewer #2254

Closed


		return val;
		} /* ecma_builtin_uint32array_dispatch_construct */


		return val;
		} /* ecma_builtin_uint8array_dispatch_construct */


		return val;
		} /* ecma_builtin_uint8clampedarray_dispatch_construct */

[ES2015][TypedArray] Add other 8 types #1532

[ES2015][TypedArray] Add other 8 types #1532

Uh oh!

Conversation

jiangzidong commented Jan 18, 2017

Uh oh!

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!

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!

jiangzidong commented Jan 18, 2017

Uh oh!

jiangzidong commented Jan 19, 2017

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!

jiangzidong Jan 20, 2017 • 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!

zherczeg commented Jan 20, 2017

Uh oh!

LaszloLango commented Jan 23, 2017

Uh oh!

jiangzidong commented Jan 23, 2017

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!

LaszloLango 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!

jiangzidong Jan 24, 2017 • 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!

LaszloLango commented Jan 24, 2017

Uh oh!

Uh oh!

jiangzidong Jan 20, 2017 •

edited

Loading

jiangzidong Jan 24, 2017 •

edited

Loading