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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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;
----------------
miyuki wrote:
> 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];
> ```
(For the record, //I// don't want to see `char_type __atoms[__num_get_base::__n_atoms_float]`; I want to see `char_type __atoms[36];`, in a separate PR, with explanation of why we need to parse `INFINITY` as a float, and appropriate tests.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99091