[libcxx-commits] [PATCH] D99091: [locale][num_get] Improve Stage 2 of string to float conversion

[Mikhail Maltsev via Phabricator via libcxx-commits]

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