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

Nikolas Klauser via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Aug 8 10:27:48 PDT 2022


philnik added inline comments.


================
Comment at: libcxx/include/__charconv/tables.h:30
 /// charconv for integrals in C++11 mode.
+#if _LIBCPP_STD_VER > 14
+// TODO(mordante) look at consolidating the tables for C++11/14 and C++17/20/2b
----------------
Mordante wrote:
> philnik wrote:
> > What is the difference between the versions? Why can't the arrays be constexpr before C++17?
> No `inline constexpr` before C++17. I want to look at this later but prefer to move that out of this change.
Isn't this in the class to achieve the same effect? Clang seems to be happy with that in C++11: https://godbolt.org/z/c6nz39xbv


================
Comment at: libcxx/include/__config:852-856
+#  if _LIBCPP_STD_VER > 20
+#    define _LIBCPP_CONSTEXPR_AFTER_CXX20 constexpr
+#  else
+#    define _LIBCPP_CONSTEXPR_AFTER_CXX20
+#  endif
----------------
Mordante wrote:
> philnik wrote:
> > I've named this `_LIBCPP_CONSTEXPR_CXX23` in D131218 because I find it a lot more intuitive. I'll open  a discussion on Discord whether we want this change or not.
> Yeah it seems we manage to get 3 reviews that basically add this macro at the same time.
> I keep it for now until we finished the discussion which direction to go.
Sounds good to me. Although there seems to be no opposition to the change.


================
Comment at: libcxx/include/charconv:720-729
+/*
+#include <cmath>
+#include <iostream>
+#include <format>
+
+int main() {
+  for (int i = 2; i <= 36; ++i)
----------------
Mordante wrote:
> philnik wrote:
> > What does this do here?
> The "script" to generate the table below.
Oh OK. Could you maybe add a comment like `// __from_chars_log2f_lut generator script`?


================
Comment at: libcxx/test/libcxx/clang_tidy.sh.cpp:11
 
+// UNSUPPORTED: gcc
+
----------------
Mordante wrote:
> @philnik does running this in GCC have any benefit for this test?
> I see there are work-arounds for unsupported diagnostics and now I have an unsupported compiler option `-fconstexpr-ops-limit`. I even wonder should this be limited to the clang compiler?
It also works for people who primarily use GCC, but I guess that's about zero people. So it's probably fine to disable it entirely for GCC. Could you also remove the compatibility stuff in that case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131317



More information about the libcxx-commits mailing list