[libcxx-commits] [PATCH] D131317: [libc++] Implements constexpr <charconv>.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/charconv:741
             using __tl = numeric_limits<_Tp>;
-            auto __digits = __tl::digits / log2f(float(__b));
+             auto __digits = __tl::digits / __from_chars_log2f_lut[__b - 2];
             _Tp __x = __in_pattern(*__p++, __b).__val, __y = 0;
----------------
Do we ever validate that `__base <= 36`? If not, we should add an assertion, but probably at a higher level. And here we should just add a comment like `// __base is always between 2 and 36 inclusive`.


================
Comment at: libcxx/test/support/charconv_test_helpers.h:99
         assert(r.ec == std::errc{});
-        assert(memcmp(buf, expect, len) == 0);
+        assert(__builtin_memcmp(buf, expect, len) == 0);
     }
----------------
I think we could use `std::equal` here?


================
Comment at: libcxx/test/support/charconv_test_helpers.h:105
     {
         using std::to_chars;
         std::to_chars_result r;
----------------
Can we get rid of this while we're at it?


================
Comment at: libcxx/test/support/charconv_test_helpers.h:141
+        if (TEST_IS_CONSTANT_EVALUATED)
+          last = const_cast<char*>(std::from_chars(p, ep, r, base).ptr);
+        else
----------------
This does weaken the test inside constexpr because this is now a roundtrip test (and we already have a test for that), but I guess it's better than nothing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131317