[libcxx-commits] [PATCH] D99091: [locale][num_get] Improve Stage 2 of string to float conversion
Mikhail Maltsev via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Apr 7 08:22:51 PDT 2021
miyuki added inline comments.
================
Comment at: libcxx/include/locale:1079
// Stage 2
- char_type __atoms[32];
+ char_type __atoms[this->__n_atoms_float];
char_type __decimal_point;
----------------
Quuxplusone wrote:
> C++ doesn't support VLAs; you'd have to put something here that's a constant-expression.
> Do I understand correctly that the majority of this patch is just changing `32` and `33` to `36` and `37` respectively? Could you just do that in the simplest possible way? E.g. here
> ```
> - char_type __atoms[32];
> + char_type __atoms[36];
> ```
> That'll help focus attention on whatever details are actually important.
>
> Meanwhile, the important change seems to be that you're adding those extra 4 characters for `"tTyY"`, so that you can parse not only `"INF"` and `"NAN"` but also `"INFINITY"` (producing `INF`). Is this required by the Standard? Why didn't we have any tests for it before now?
>
> I tentatively suggest that you split out the `"INFINITY"` change+test into its own PR (with a summary that cites chapter and verse for why this is needed); and then let's see what remains in this PR after that.
I guess
```
char_type __atoms[__num_get_base::__n_atoms_float];
```
will work. We already have something similar on line 1089:
```
unsigned __g[__num_get_base::__num_get_buf_sz];
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D99091/new/
https://reviews.llvm.org/D99091
More information about the libcxx-commits
mailing list