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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon May 17 08:08:00 PDT 2021

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
+    // Valid floating point formats where whole string is consumed
Is this ever defined during testing, though?

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list