[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 a comment.

I left some more minor comments... but FYI, this patch's actual functional change is above my pay grade. You'll have to interest @ldionne or someone like that in reviewing its actual functional change.
If the intent is speed, it would help for you to add to this PR a benchmark following the pattern in `libcxx/benchmarks/`.



================
Comment at: libcxx/include/locale:594
+{
+#define __UPPERCASE(__x) ((__x)&0x5F)
+
----------------
`#define _Toupper(x) ((x) & 0x5F)` — "`__UPPERCASE`" reads to me like it's //testing// uppercaseness.

Separately: check throughout for `/preceed/preced/`.


================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/facet.num.get.members/get_float.pass.cpp:50
+    TEST("-inF", 4, -INFINITY, ios.goodbit | ios.eofbit);
+    TEST("INFxyz", 3, INFINITY, ios.goodbit);
+
----------------
I'd still like to see the test strings `"INFINITE"` and `"INFINITY"` somewhere in these tests. Also `INAN`, now that I think of it (because it looks like it's gonna be "INF" and then switches to "NAN" in the middle) — and any other "white-box" test cases you can think of.
Also every-possible-prefix-of-a-correct string: `0x`, `123e`, `123e+`, `123e-`. (Maybe these are already covered somewhere?)


================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/facet.num.get.members/get_long_double.pass.cpp:43
+
+#ifdef _LIBCPP_ABI_LOCAL_NUM_GET_NEW_STAGE2_FLOAT_LOOP
+    // Valid floating point formats where whole string is consumed
----------------
Is this ever defined during testing, though?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99091