[libc-commits] [PATCH] D113036: [libc] refactor atof string parsing

[Tue Ly via Phabricator via libc-commits]

lntue accepted this revision.
lntue added a comment.
This revision is now accepted and ready to land.

Overall the patch looks good for me.  I can see where the possible rounding errors come from and also we can definitely improve to support parsing longer strings than fitting into the integer types.  But we can leave them for the followups.



================
Comment at: libc/src/__support/str_to_float.h:460
+  int32_t amountToShift =
+      (BITS_IN_BITSTYPE - fputil::FloatProperties<T>::mantissaWidth - 1) -
+      leadingZeroes<BitsType>(mantissa);
----------------
BITS_IN_BITSTYPE is only used in this expression, and this part is also constexpr.  So maybe merge this into the BITS_IN_BITSTYPE definition and change its name appropriately?


================
Comment at: libc/src/__support/str_to_float.h:579-586
+    if (*src == DECIMAL_POINT && afterDecimal) {
+      break; // this means that *src points to a second decimal point, ending
+             // the number.
+    } else if (*src == DECIMAL_POINT) {
+      afterDecimal = true;
+      ++src;
+      continue;
----------------
What do you think about restructuring this part to:

  if (*src == DECIMAL_POINT) {
    if (afterDecimal) {
      // comments
      break;
    }
    afterDecimal = true;
    ++src;
    continue;
  }


================
Comment at: libc/src/__support/str_to_float.h:608
+
+      src += tempStrEnd - src;
+      exponent += add_to_exponent;
----------------
Is this the same as src = tempStrEnd?


================
Comment at: libc/src/__support/str_to_float.h:654-661
+    if (*src == DECIMAL_POINT && afterDecimal) {
+      break; // this means that *src points to a second decimal point, ending
+             // the number.
+    } else if (*src == DECIMAL_POINT) {
+      afterDecimal = true;
+      ++src;
+      continue;
----------------
Similar structure as the above comment for decimal?


================
Comment at: libc/src/__support/str_to_float.h:666
+
+    mantissa = (mantissa * BASE) + digit;
+    seenDigit = true;
----------------
Does it mean that we cannot parse the string longer than 16 hexadecimal digits?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113036/new/

https://reviews.llvm.org/D113036