[libcxx-commits] [PATCH] D128139: [libc++][format] Improve integral formatters.

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Jun 20 12:06:57 PDT 2022

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

This patch seems to be moving around quite a bit of code. Would it be possible to do some of the reorganization before this patch as a NFC to ease reviewing this?

Comment at: libcxx/include/__format/formatter_bool.h:55
+    case __format_spec::__type::__hexadecimal_upper_case:
+      // Promotes _CharT to an integral type. This reduces the number of
+      // instantiations of __format_integer reducing code size.

Comment at: libcxx/include/__format/formatter_integer.h:24
+#include <limits>      // TODO FMT Remove after adding 128-bit support
+#include <type_traits> // is_signed_v

Comment at: libcxx/include/__format/formatter_integer.h:53
+  template <integral _Tp>
+  static _LIBCPP_HIDE_FROM_ABI auto format(_Tp __value, auto& __ctx,
+                                           __format_spec::__parsed_specifications<_CharT> __specs)
Let's call this `__format_impl` or something like that, to make it clear that it's not part of the public API.

Comment at: libcxx/include/__format/formatter_integral.h:120
+      if (__value < numeric_limits<_CharT>::min() || __value > numeric_limits<_CharT>::max())
+        __throw_format_error("Integral value outside the range of the char type");
+    } else if constexpr (signed_integral<_CharT>) {
ADL proofing is technically not needed because we call it with `char const*`, but I would rather be consistent since it's not a member function.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list