[libcxx-commits] [PATCH] D128215: [libc++] Reduces std::to_chars instantiations.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 6 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/charconv:491
+  using __type = __make_32_64_or_128_bit_t<_Tp>;
+  static_assert(!is_same<__type, void>::value || sizeof(_Tp) > sizeof(int64_t), "unsupported integral type used in to_chars");
+  return __to_chars_itoa(__first, __last, static_cast<__type>(__value), is_signed<_Tp>());
----------------
ldionne wrote:
> I think you need parentheses here.
For simplicity I just split it into two asserts, that makes removing 128-bit one easier.


================
Comment at: libcxx/include/type_traits:1024-1045
+template <class _Tp>
+using __make_32_64_or_128_bit_t = typename conditional<
+    is_signed<_Tp>::value,
+    typename conditional<
+        sizeof(_Tp) <= sizeof(int32_t), int32_t,
+        typename conditional<sizeof(_Tp) <= sizeof(int64_t), int64_t,
+#ifndef _LIBCPP_HAS_NO_INT128
----------------
ldionne wrote:
> Mordante wrote:
> > philnik wrote:
> > > Could you put this into it's own header?
> > Yes I think that makes sense, but I first want to settle on a good name.
> I would suggest this instead (the reformatting would make it easier to read IMO):
> 
> ```
> template <bool _Cond, class _If, class _Else>
> using __conditional_t = typename conditional<_Cond, _If, _Else>::type;
> 
> template <class _Tp>
> using __make_32_64_or_128_bit_t = __copy_unsigned_t<_Tp,
>   __conditional_t<sizeof(_Tp) <= sizeof(int32_t),   int32_t,
>   __conditional_t<sizeof(_Tp) <= sizeof(int64_t),   int64_t,
>   __conditional_t<sizeof(_Tp) <= sizeof(__int128_t), __int128_t,
>   /* else */                                         void
>   >>>
> >;
> ```
> 
> Here, `__copy_unsigned_t ` would be basically
> 
> ```
> template <class _Tp, class _Up>
> using __copy_unsigned_t = typename conditional<is_unsigned<_Tp>::value, make_unsigned_t<_Up>, _Up>;
> ```
After the rewrite to the suggestion above that's no longer an option; it uses `make_unsigned`. Do you have patches in progress to move that? If not I can make a followup patch to move `make_unsigned` and this new helper.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128215