[libc-commits] [PATCH] D109261: [libc][WIP] add atof, strtof, strtod, and strtold

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

Just did a quick pass. Will do a detailed review when it is final.



================
Comment at: libc/src/__support/detailed_powers_of_ten.h:18
+// This table was generated by
+// https://github.com/google/wuffs/blob/788479dd64f35cb6b4e998a851acb06ee962435b/script/print-mpb-powers-of-10.go
+// and contains the 128 bit mantissa approximations of the powers of 10 from
----------------
We should ideally have the script which produced these constants checked in. I encourage you to look at https://www.sollya.org/ using which one can trivially produce these constants. We can check-in sollya scripts. For example: https://github.com/llvm/llvm-project/blob/main/libc/utils/mathtools/expm1f.sollya


================
Comment at: libc/src/__support/str_conv_utils.h:242
+
+  fputil::UInt<128> firstApprox = fputil::UInt<128>(mantissa);
+  firstApprox.mul(powerOfTen[1]);
----------------
Using `fputil:UInt` is probably an overkill here if all you want is a 128-bit integer. You should probably use `__uint128_t`. For the access to the individual 32-bits words, you can use a `MutableArrayRef`: https://github.com/llvm/llvm-project/blob/main/libc/utils/CPP/ArrayRef.h#L123.

So, something like this:

```
__uint128_t finalApprox = mantissa;
MutableArrayRef<uint32_t> finalApproxWords(reinterpret_cast<uint32_t *>(&finalApprox, 4);
```

To reduce verbosity, you can may be define:

```
using UInt128Words = MutableArrayRef<uint32_t>;
static inline UInt128Words makeUInt128Words(__uint128_t *val) {
  return UInt128Words(reinterpret_cast<uint32_t *>(val), 4);
}
```

That said, I will leave it up to you to decide which is more readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109261