[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:524
+
+  if (__ct == __decimal_point) {
+    if (!__in_units)
----------------
tmatheson wrote:
> miyuki wrote:
> > I think it would be better to keep 4-space indentation for consistency with the rest of the file. See https://github.com/llvm/llvm-project/commit/7004d6664efde9d1148ed677649593f989cc6056
> I agree, but the CI was failing if I didn't clang-format the patch.
The clang-format CI step is non-fatal; it's just there to tell you what parts fail clang-format. It is not intended as a black-and-white gatekeeper, and requests-for-consistency from real humans should always take precedence over the tool's suggestions — [Asimov's Second Law](https://en.wikipedia.org/wiki/Three_Laws_of_Robotics) applies. :)



================
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);
+
----------------
tmatheson wrote:
> Quuxplusone wrote:
> > `INFxyz`, `INFinity`, `INFinite`, `INFiNiTy`
> Added INFxyz and some crazy casing for INF.
> 
> It is worth noting that (by my reading of the standard, at least) `INFinite` is required to fail because:
> 
> * Stage 1 will select `%g` as the format specifier
> * Stage 2 will keep consuming characters until it reaches the `'e'`, which is the first character that is not valid at that point in the sequence
> * Stage 3 will then try to process the strong `"INFinit"` and should only process the first 3 characters (`INF`). Therefore the whole string will not be read, and `num_get` should return 0.0 with `failbit`
> 
> I suggest we save that discussion for the INFINITE PR though.
The point of testing `"INFINITE"` is for two reasons:
- There is a right answer. Test that we produce the right answer.
- It's a "garden-path" input: we want to test that the parser doesn't get confused by seeing "INFINI...", and will correctly backtrack to consume only the "INF" part, instead of failing when it fails to find a "Y" character at the end of the string.
So, please test its behavior (in the appropriate PR which it sounds like you're splitting out; nice!). I expect that behavior to resemble the behavior for `"INFxyz"`; but if it doesn't, then we should investigate why.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99091