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

[Tomas Matheson via Phabricator via libcxx-commits]

tmatheson marked 2 inline comments as done.
tmatheson added inline comments.


================
Comment at: libcxx/include/locale:524
+
+  if (__ct == __decimal_point) {
+    if (!__in_units)
----------------
Quuxplusone wrote:
> 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. :)
> 
Sorry my mistake, I thought it had caused one of the failures I had earlier


================
Comment at: libcxx/test/std/localization/locale.categories/category.numeric/locale.num.get/facet.num.get.members/get_float.pass.cpp:55
-    {
-        const char str[] = "-123";
-        std::ios_base::iostate err = ios.goodbit;
----------------
miyuki wrote:
> I think there is a reason behind having many similar-looking blocks: each assert() call is located on a separate line, so an assertion failure message will indicate which assertion failed. Factoring out all checks into a single function will make error messages much less informative.
This was addressed by using a macro for new tests and not refactoring the old tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99091