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

[Louis Dionne via Phabricator via libcxx-commits]

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128139