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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 3 inline comments as done.
Mordante added a comment.

Thanks for the review!



================
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
----------------
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.


================
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
----------------
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.


================
Comment at: libcxx/include/charconv:33
 
-  to_chars_result to_chars(char* first, char* last, see below value,
-                           int base = 10);
+  constexpr to_chars_result to_chars(char* first, char* last, see below value,
+                                     int base = 10);                                  // constexpr since C++23
----------------
philnik wrote:
> I think we don't add the `constexpr` if it's "constexpr since C++ab".
We do, `git grep 'constexpr.*constexpr' include` shows a lot of hits, some for C++14.


================
Comment at: libcxx/include/charconv:720-729
+/*
+#include <cmath>
+#include <iostream>
+#include <format>
+
+int main() {
+  for (int i = 2; i <= 36; ++i)
----------------
philnik wrote:
> What does this do here?
The "script" to generate the table below.


================
Comment at: libcxx/include/charconv:752
+#if _LIBCPP_STD_VER > 14
+             auto __digits = __tl::digits / __from_chars_log2f_lut[__b - 2];
+#else
----------------
philnik wrote:
> Is there a reason this isn't in C++11 or 14?
yes not `inline constexpr` and `log2f` itself isn't `constexpr` so it can't be used in C++23.


================
Comment at: libcxx/test/libcxx/clang_tidy.sh.cpp:11
 
+// UNSUPPORTED: gcc
+
----------------
@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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131317