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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/locale:1079
     // Stage 2
-    char_type __atoms[32];
+    char_type __atoms[this->__n_atoms_float];
     char_type __decimal_point;
----------------
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.


================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/facet.num.get.members/get_double.pass.cpp:57
+    TEST("inf", 3, INFINITY, ios.goodbit | ios.eofbit);
+    TEST("INFINITY", 8, INFINITY, ios.eofbit | ios.goodbit);
+
----------------
`INFxyz`, `INFinity`, `INFinite`, `INFiNiTy`


================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/facet.num.get.members/get_double.pass.cpp:66
+
+    // Should't recognise e, p or x more than once
+    TEST("123.4e-5e-4", 8, 123.4e-5, ios.goodbit);
----------------
`Shouldn't`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99091