Clean up radix conversion in Number toString method (#2002)

The radix conversion code path was very messy which made it hard to understand
what was happening inside of it. The code got cleaned up, and a lot of comments
were added that explain what is happening and why.

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai@inf.u-szeged.hu

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

@@ -266,90 +266,132 @@ ecma_builtin_number_prototype_object_to_string (ecma_value_t this_arg, /**< this
     }
     else
     {
-      bool is_negative = false;
+      int buff_size = 0;
+
+      bool is_number_negative = false;
       if (ecma_number_is_negative (this_arg_number))
       {
+        /* ecma_number_to_decimal can't handle negative numbers, so we get rid of the sign. */
         this_arg_number = -this_arg_number;
-        is_negative = true;
+        is_number_negative = true;
+
+        /* Add space for the sign in the result. */
+        buff_size += 1;
       }
 
+      /* Decompose the number. */
       lit_utf8_byte_t digits[ECMA_MAX_CHARS_IN_STRINGIFIED_NUMBER];
       int32_t exponent;
-      lit_utf8_size_t num_digits = ecma_number_to_decimal (this_arg_number, digits, &exponent);
+      lit_utf8_size_t digit_count = ecma_number_to_decimal (this_arg_number, digits, &exponent);
 
-      exponent = exponent - (int32_t) num_digits;
+      /*
+       * The 'exponent' given by 'ecma_number_to_decimal' specifies where the decimal point is located
+       * compared to the first digit in 'digits'.
+       * For example: 120 -> '12', exp: 3 and 0.012 -> '12', exp: -1
+       * We convert it to be location of the decimal point compared to the last digit of 'digits':
+       * 120 -> 12 * 10^1 and 0.012 -> 12 * 10^-3
+       */
+      exponent = exponent - (int32_t) digit_count;
 
-      /* Calculate the scale of the number in the specified radix. */
+      /* 'magnitude' will be the magnitude of the number in the specific radix. */
-      int scale = (int) -floor ((log (10) / log (radix)) * exponent);
+      int magnitude;
-
+      int required_digits;
-      bool is_scale_negative = false;
+      if (exponent >= 0)
-      if (scale < 0)
-      {
-        is_scale_negative = true;
-        scale = -scale;
-      }
-
-      int buff_size = 1;
-      if (is_scale_negative || scale == 0)
       {
+        /*
+         * If the exponent is non-negative that means we won't have a fractional part, and can calculate
+         * exactly how many digits we will have. This could be done via a mathematic formula, but in rare
+         * cases that can cause incorrect results due to precision issues, so we use a loop instead.
+         */
+        magnitude = 0;
         double counter = this_arg_number;
         while (counter >= radix)
         {
           counter /= radix;
-          buff_size++;
+          magnitude++;
         }
+
+        /*
+         * The magnitude will only tell us how many digits we have after the first one, so we add one extra.
+         * In this case we won't be needing a radix point, so we don't need to worry about space for it.
+         */
+        required_digits = magnitude + 1;
       }
       else
       {
-        buff_size = scale + ECMA_NUMBER_FRACTION_WIDTH + 2;
+        /*
+         * We can't know exactly how many digits we will need, since the number may be non-terminating in the
+         * new radix, so we will have to estimate it. We do this by first calculating how many zeros we will
+         * need in the specific radix before we hit a significant digit. This is calculated from the decimal
+         * exponent, which we negate so that we get a positive number in the end.
+         */
+        magnitude = (int) floor ((log (10) / log (radix)) * -exponent);
+
+        /*
+         * We also need to add space for significant digits. The worst case is radix == 2, since this will
+         * require the most digits. In this case, the upper limit to the number of significant digits we can have is
+         * ECMA_NUMBER_FRACTION_WIDTH + 1. This should be sufficient for any number.
+         */
+        required_digits = magnitude + ECMA_NUMBER_FRACTION_WIDTH + 1;
+
+        /*
+         * We add an exta slot for the radix point. It is also likely that we will need extra space for a
+         * leading zero before the radix point. It's better to add space for that here as well, even if we may not
+         * need it, since later we won't be able to do so.
+         */
+        buff_size += 2;
       }
 
-      if (is_negative)
+      /*
+       * Here we normalize the number so that it is as close to 0 as possible, which will prevent us from losing
+       * precision in case of extreme numbers when we later split the number into integer and fractional parts.
+       * This has to be done in the specific radix, otherwise it messes up the result, so we use magnitude instead.
+       */
+      if (exponent > 0)
       {
-        buff_size++;
+        for (int i = 0; i < magnitude; i++)
-      }
-
-      /* Normalize the number, so that it is as close to 0 exponent as possible. */
-      if (is_scale_negative)
-      {
-        for (int i = 0; i < scale; i++)
         {
           this_arg_number /= (ecma_number_t) radix;
         }
       }
-      else
+      else if (exponent < 0)
       {
-        for (int i = 0; i < scale; i++)
+        for (int i = 0; i < magnitude; i++)
         {
           this_arg_number *= (ecma_number_t) radix;
         }
       }
 
+      /* Split the number into an integer and a fractional part, since we have to handle them separately. */
       uint64_t whole = (uint64_t) this_arg_number;
       ecma_number_t fraction = this_arg_number - (ecma_number_t) whole;
 
       bool should_round = false;
-      if (!ecma_number_is_zero (fraction) && is_scale_negative)
+      if (!ecma_number_is_zero (fraction) && exponent >= 0)
       {
-        /* Add one extra digit for rounding. */
+        /*
-        buff_size++;
+         * If the exponent is non-negative, and we get a non-zero fractional part, that means
+         * the normalization might have introduced a small error, in which case we have to correct it by rounding.
+         * We'll add one extra significant digit which we will later use to round.
+         */
+        required_digits += 1;
         should_round = true;
       }
 
+      /* Get the total required buffer size and allocate the buffer. */
+      buff_size += required_digits;
       JMEM_DEFINE_LOCAL_ARRAY (buff, buff_size, lit_utf8_byte_t);
       int buff_index = 0;
 
       /* Calculate digits for whole part. */
       while (whole > 0)
       {
+        JERRY_ASSERT (buff_index < buff_size && buff_index < required_digits);
         buff[buff_index++] = (lit_utf8_byte_t) (whole % radix);
         whole /= radix;
       }
 
-      /* Calculate where we have to put the radix point. */
+      /* The digits are backwards, we need to reverse them. */
-      int point = is_scale_negative ? buff_index + scale : buff_index - scale;
-
-      /* Reverse the digits, since they are backwards. */
       for (int i = 0; i < buff_index / 2; i++)
       {
         lit_utf8_byte_t swap = buff[i];
@@ -357,20 +399,26 @@ ecma_builtin_number_prototype_object_to_string (ecma_value_t this_arg, /**< this
         buff[buff_index - i - 1] = swap;
       }
 
-      int required_digits = buff_size;
+      /*
-      if (is_negative)
+       * Calculate where we have to put the radix point relative to the beginning of
+       * the new digits. If the exponent is non-negative this will be right after the number.
+       */
+      int point = exponent >= 0 ? magnitude + 1: buff_index - magnitude;
+
+      if (point < 0)
       {
-        required_digits--;
+        /*
+         * In this case the radix point will be before the first digit,
+         * so we need to leave space for leading zeros.
+         */
+        JERRY_ASSERT (exponent < 0);
+        required_digits += point;
       }
 
-      if (!is_scale_negative)
+      JERRY_ASSERT (required_digits <= buff_size);
-      {
-        /* Leave space for leading zeros / radix point. */
-        required_digits -= scale + 1;
-      }
 
       /* Calculate digits for fractional part. */
-      while (buff_index < required_digits && (fraction != 0 || is_scale_negative))
+      while (buff_index < required_digits)
       {
         fraction *= (ecma_number_t) radix;
         lit_utf8_byte_t digit = (lit_utf8_byte_t) floor (fraction);
@@ -381,31 +429,35 @@ ecma_builtin_number_prototype_object_to_string (ecma_value_t this_arg, /**< this
 
       if (should_round)
       {
-        /* Round off last digit. */
+        /* Consume last digit for rounding. */
-        if (buff[buff_index - 1] > radix / 2)
-        {
-          buff[buff_index - 2]++;
-        }
-
         buff_index--;
-
+        if (buff[buff_index] > radix / 2)
-        /* Propagate carry. */
-        for (int i = buff_index - 1; i > 0 && buff[i] >= radix; i--)
         {
-          buff[i] = (lit_utf8_byte_t) (buff[i] - radix);
+          /* We should be rounding up. */
-          buff[i - 1]++;
+          buff[buff_index - 1]++;
-        }
 
-        /* Carry propagated over the whole number, need to add a leading digit. */
+          /* Propagate carry forward in the digits. */
-        if (buff[0] >= radix)
+          for (int i = buff_index - 1; i > 0 && buff[i] >= radix; i--)
-        {
+          {
-          memmove (buff + 1, buff, (size_t) buff_index);
+            buff[i] = (lit_utf8_byte_t) (buff[i] - radix);
-          buff_index++;
+            buff[i - 1]++;
-          buff[0] = 1;
+          }
+
+          if (buff[0] >= radix)
+          {
+            /*
+             * Carry propagated over the whole number, we need to add a new leading digit.
+             * We can use the place of the original rounded digit, we just need to shift everything
+             * right by one.
+             */
+            memmove (buff + 1, buff, (size_t) buff_index);
+            buff_index++;
+            buff[0] = 1;
+          }
         }
       }
 
-      /* Remove trailing zeros from fraction. */
+      /* Remove trailing zeros. */
       while (buff_index - 1 > point && buff[buff_index - 1] == 0)
       {
         buff_index--;
@@ -414,14 +466,17 @@ ecma_builtin_number_prototype_object_to_string (ecma_value_t this_arg, /**< this
       /* Add leading zeros in case place of radix point is negative. */
       if (point <= 0)
       {
-        memmove (buff - point + 1, buff, (size_t) buff_index);
+        /* We will have 'point' amount of zeros after the radix point, and +1 before. */
-        buff_index += -point + 1;
+        int zero_count = -point + 1;
+        memmove (buff + zero_count, buff, (size_t) buff_index);
+        buff_index += zero_count;
 
-        for (int i = 0; i < -point + 1; i++)
+        for (int i = 0; i < zero_count; i++)
         {
           buff[i] = 0;
         }
 
+        /* We now need to place the radix point after the first zero. */
         point = 1;
       }
 
@@ -440,11 +495,11 @@ ecma_builtin_number_prototype_object_to_string (ecma_value_t this_arg, /**< this
       }
 
       /* Add negative sign if necessary. */
-      if (is_negative)
+      if (is_number_negative)
       {
         memmove (buff + 1, buff, (size_t) buff_index);
-        buff_index++;
         buff[0] = '-';
+        buff_index++;
       }
 
       JERRY_ASSERT (buff_index <= buff_size);

tests/jerry/number-prototype-to-string.js

@@ -90,7 +90,7 @@ assert ((-0x100000000000061).toString(16) === "-100000000000060");
 
 assert((123400).toString(new Number(16)) === "1e208");
 
-assert(65535.9.toString(3) === "10022220020.220022002200220022002210211012200000110221");
+assert(65535.9.toString(3) === "10022220020.2200220022002200220022102110122000001102212");
 
 var digit_chars = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
                    'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j',