Implemented Number.prototype.toFixed() #100

tczene · 2015-05-26T13:54:18Z

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
JerryScript-DCO-1.0-Signed-off-by: Tamas Czene tczene.u-szeged@partner.samsung.com

galpeter · 2015-05-26T14:36:52Z

jerry-core/ecma/base/ecma-helpers.h

@@ -312,6 +312,7 @@ extern int32_t ecma_number_to_int32 (ecma_number_t value);
 extern ecma_number_t ecma_int32_to_number (int32_t value);
 extern ecma_number_t ecma_uint32_to_number (uint32_t value);
 extern ecma_length_t ecma_number_to_zt_string (ecma_number_t num, ecma_char_t *buffer_p, ssize_t buffer_size);
+void ecma_number_to_zt_string_calc_number_params (ecma_number_t num, uint64_t *out_digits_p, int32_t *out_digits_num_p, int32_t *out_decimal_exp_p);


Shouldn't this be extern like the others in this case?

egavrin · 2015-05-27T09:10:47Z

jerry-core/ecma/base/ecma-helpers-conversion.cpp

@@ -932,7 +931,7 @@ ecma_number_to_zt_string_to_decimal (ECMA_NUMBER_CONVERSION_128BIT_INTEGER_ARG (
 /**
 * Calculate s, n and k parameters for specified ecma-number according to ECMA-262 v5, 9.8.1, item 5
 */
-static void


Are there any reason to remove static?

If the function stays static I cannot call it.

egavrin · 2015-05-27T09:34:49Z

jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp

+    digits++;
+  }
+
+  /* 8. */


General comment on this if/else block. Functionally it seams OK, but I think we may introduce several helper functions to improve readability.

egavrin · 2015-05-27T17:23:38Z

+ make precommit -j4
./jerry-core/ecma/base/ecma-helpers.h:315: error: line is longer than 120 characters
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:264: error: there should be exactly one space before left parentheses

ruben-ayrapetyan · 2015-05-28T12:36:31Z

jerry-core/ecma/base/ecma-helpers-number.cpp

@@ -928,6 +928,27 @@ ecma_number_exp (ecma_number_t num) /**< valid finite number */
  return sum;
 } /* ecma_number_exp */

+void
+ecma_number_round_last_digit (uint64_t &digits, int32_t num_digits, int32_t exponent, int32_t precision)


Could you, please, add comments to functions, and their arguments?

egavrin · 2015-05-29T13:17:32Z

https://github.com/Samsung/jerryscript/commit/3cc6d85adc6695e6b283d31d7ead38c6b9afebbd
+ make precommit -j4
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:199: error: Indentation: 6 -> 4. Line: '      if (ecma_number_is_nan (this_num))'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:200: error: Indentation: 6 -> 4. Line: '      {'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:201: error: Indentation: 8 -> 6. Line: '        ecma_string_t *nan_str_p = ecma_get_magic_string (ECMA_MAGIC_STRING_NAN);'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:202: error: Indentation: 8 -> 6. Line: '        ret_value = ecma_make_normal_completion_value (ecma_make_string_value (nan_str_p));'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:203: error: Indentation: 6 -> 4. Line: '      }'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:205: error: Indentation: 6 -> 4. Line: '      else if (ecma_number_is_negative (this_num))'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:206: error: Indentation: 6 -> 4. Line: '      {'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:207: error: Indentation: 8 -> 6. Line: '        is_negative = true;'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:208: error: Indentation: 8 -> 6. Line: '        this_num *= -1;'
./jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp:209: error: Indentation: 6 -> 4. Line: '      }'

ruben-ayrapetyan · 2015-06-01T09:29:26Z

jerry-core/ecma/base/ecma-helpers.h

+/**
+ * Declare 128-bit integer.
+ */
+#define ECMA_NUMBER_CONVERSION_128BIT_INTEGER(name) uint64_t name[4] = { 0, 0, 0, 0 }


Why do we need this in the header?

I moved this part to share with ecma-helpers-number.cpp file.

egavrin · 2015-06-01T11:44:23Z

@tczene could you, please, answer to all questions in the original review, it seems that some of notes were missed.

egavrin · 2015-06-02T16:10:07Z

@galpeter could you please recheck this issue?

galpeter · 2015-06-08T11:07:39Z

I've got no other comments. lgtm

galpeter · 2015-06-10T12:31:51Z

@egavrin anything to add? or it is okay to make push this when there is time for it?

ruben-ayrapetyan · 2015-06-15T21:51:26Z

@tczene,

I see that with the changes there are two different functions, both performing conversion of number to decimal representation with decimal exponent. Do I understand it correctly?

If it is correct, it seems that there are two functions that use ECMA_NUMBER_CONVERSION_128BIT for performing the same or very similar operations. Usage of the ECMA_NUMBER_CONVERSION_128BIT increases code size significantly.

Could we somehow extract the conversion algorithms used in both functions with lightweight part for FLOAT32-mode and precise part for FLOAT64-mode, putting them into separate helper routine, and just call the helper from both functions. In the case, we could also move ECMA_NUMBER_CONVERSION_128BIT from header back to module, containing the helper.

What do you think about this?

dbatyai · 2015-06-17T08:26:57Z

@ruben-ayrapetyan,

You're correct in that we're using 128bit integers in two functions, but the operations they perform are the opposite of each other. In ecma_zt_string_to_number we use them to create a number from a fraction and exponent part, and in ecma_number_calc_params we use them to break down a number into a fraction and exponent. (Actually, they're also used in ecma_number_to_decimal, but that is just a helper for ecma_number_calc_params, and is only called from here. )

Both of these functions already have a lightweight FLOAT32 part, and a precise FLOAT64 part. 128 bit integers are only used in case of FLOAT64 in both functions, so in my opinion their use is already fairly localized. We could move the macros for 128bit integers back to ecma-helpers-conversion.cpp, and use them from there, if that is of concern.

Also, if we are concerned with code size, we should probably consider changing some, or even all of the macros into helper functions, so that their impact on code size is less.

ruben-ayrapetyan · 2015-06-18T14:29:04Z

@dbatyai, thank you very much for your explanation.

dbatyai · 2015-06-18T14:35:44Z

@ruben-ayrapetyan,
In the meantime, I've made some changes to this patch.
I moved the 128bit integer macros back to ecma-helpers-conversion, so that we don't move them unneccessarily. This way, the actual changes we made should also be easier to notice.

ruben-ayrapetyan · 2015-06-18T14:44:29Z

jerry-core/ecma/base/ecma-helpers-conversion.cpp

+ecma_number_calc_params (ecma_number_t num, /**< ecma-number */
+                         uint64_t *out_digits_p, /**< out: digits */
+                         int32_t *out_digits_num_p, /**< out: number of digits */
+                         int32_t *out_decimal_exp_p) /**< out: decimal exponent */


@dbatyai, could you, please, rename the ecma_number_calc_params function to ecma_number_to_decimal, and the current ecma_number_to_decimal to ecma_number_helper_binary_to_decimal?

Could you, please, also update comments to the functions?

new ecma_number_to_decimal:

/** * Perform conversion of ecma-number to decimal representation with decimal exponent * * Note: * The calculated values correspond to s, n, k parameters in ECMA-262 v5, 9.8.1, item 5: * - s represents digits of the number; * - k is the number of digits; * - n is the decimal exponent. */

ecma_number_helper_binary_to_decimal:

/** * Perform conversion of 128-bit binary representation of number * to decimal representation with decimal exponent. */

Thank you.

ruben-ayrapetyan · 2015-06-18T14:47:23Z

In the meantime, I've made some changes to this patch.
I moved the 128bit integer macros back to ecma-helpers-conversion, so that we don't move them unneccessarily. This way, the actual changes we made should also be easier to notice.

Thank you for making the changes.

dbatyai · 2015-06-19T08:55:37Z

I've updated the patch.

dbatyai · 2015-06-19T10:43:29Z

@egavrin, @ruben-ayrapetyan,

Do you have any other comments, or may we merge this?

ruben-ayrapetyan · 2015-06-19T11:49:24Z

jerry-core/ecma/builtin-objects/ecma-builtin-number-prototype.cpp

+  int32_t frac_digits = 0;
+
+  /* 1. */
+  if (!ecma_is_value_undefined (arg))


The check seems to be unnecessary, as ToNumber returns NaN for undefined, and ToInt32 returns 0 for NaN.

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com JerryScript-DCO-1.0-Signed-off-by: Tamas Czene tczene.u-szeged@partner.samsung.com

dbatyai · 2015-06-19T12:15:53Z

@ruben-ayrapetyan , updated.

ruben-ayrapetyan · 2015-06-19T12:20:03Z

Looks good to me.

galpeter · 2015-06-19T14:06:51Z

seems good to me also.

egavrin · 2015-06-19T15:06:59Z

make push

dbatyai · 2015-06-19T19:02:04Z

merged: 4836d3b

galpeter reviewed May 26, 2015
View reviewed changes

galpeter added the ecma builtins Related to ECMA built-in routines label May 27, 2015

galpeter added this to the ECMA builtins milestone May 27, 2015

egavrin reviewed May 27, 2015
View reviewed changes

egavrin self-assigned this May 27, 2015

egavrin reviewed May 27, 2015
View reviewed changes

galpeter mentioned this pull request May 27, 2015

Implement ECMA262 Number builtin object #83

Closed

16 tasks

ruben-ayrapetyan reviewed May 28, 2015
View reviewed changes

ruben-ayrapetyan reviewed Jun 1, 2015
View reviewed changes

egavrin assigned galpeter and unassigned egavrin Jun 5, 2015

egavrin assigned egavrin and unassigned galpeter Jun 8, 2015

dbatyai mentioned this pull request Jun 18, 2015

Implement printing in the specified radix for Number.prototype.toString(). #207

Closed

ruben-ayrapetyan reviewed Jun 18, 2015
View reviewed changes

ruben-ayrapetyan assigned dbatyai and unassigned ruben-ayrapetyan Jun 18, 2015

ruben-ayrapetyan reviewed Jun 19, 2015
View reviewed changes

ruben-ayrapetyan assigned egavrin and unassigned dbatyai Jun 19, 2015

dbatyai closed this Jun 19, 2015

somang-park unassigned egavrin Nov 25, 2016

This was referenced May 17, 2020

stack-overflow in vm_loop #3750

Closed

stack-overflow in ecma_regexp_match #3753

Closed

Implemented Number.prototype.toFixed() #100

Implemented Number.prototype.toFixed() #100

Uh oh!

Conversation

tczene commented May 26, 2015

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!

egavrin commented May 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egavrin commented May 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egavrin commented Jun 1, 2015

Uh oh!

egavrin commented Jun 2, 2015

Uh oh!

galpeter commented Jun 8, 2015

Uh oh!

galpeter commented Jun 10, 2015

Uh oh!

ruben-ayrapetyan commented Jun 15, 2015

Uh oh!

dbatyai commented Jun 17, 2015

Uh oh!

ruben-ayrapetyan commented Jun 18, 2015

Uh oh!

dbatyai commented Jun 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruben-ayrapetyan commented Jun 18, 2015

Uh oh!

dbatyai commented Jun 19, 2015

Uh oh!

dbatyai commented Jun 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbatyai commented Jun 19, 2015

Uh oh!

ruben-ayrapetyan commented Jun 19, 2015

Uh oh!

galpeter commented Jun 19, 2015

Uh oh!

egavrin commented Jun 19, 2015

Uh oh!

dbatyai commented Jun 19, 2015

Uh oh!

Uh oh!