[libcxx-commits] [PATCH] D129931: [libc++][format] Use to_chars internals.

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks for the patch! I think the improvement is nice, but I have a few concerns about duplicating logic with `to_chars`.



================
Comment at: libcxx/include/__format/formatter_integral.h:257-271
+template <int _Base, unsigned_integral _Tp>
+char* __set_prefix(char* __begin, _Tp __value, bool __upper_case) {
+  *__begin = '0';
+  if constexpr (_Base == 2) {
+    *(__begin + 1) = __upper_case ? 'B' : 'b';
+    return __begin + 2;
+  } else if constexpr (_Base == 8) {
----------------



================
Comment at: libcxx/include/__format/formatter_integral.h:293
+template <unsigned_integral _Tp>
+char* __to_chars_base_10(char* __first, _Tp __value) {
+  if constexpr (sizeof(_Tp) <= sizeof(uint32_t))
----------------
Why is this required? Wouldn't it be possible to instead call a single `__to_chars_base` function that contains this logic (but it would live inside the implementation of `to_chars`).

I think what I'm saying is that I don't understand why `__to_chars_integral` doesn't handle the various bases itself. IMO it should be possible to easily bypass the checks you're trying to avoid in `to_chars` without reimplementing this base dispatching.


================
Comment at: libcxx/include/__format/formatter_integral.h:311
+template <unsigned_integral _Tp, class _CharT>
+_LIBCPP_HIDE_FROM_ABI auto __format_integer_base_10(
+    _Tp __value, auto& __ctx, __format_spec::__parsed_specifications<_CharT> __specs, bool __negative)
----------------
Would it be possible to handle base 10 in `__format_integer_base` with `if constexpr` instead? I'm trying to find ways to make the switch below look more regular.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129931