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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
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>());
----------------
I think you need parentheses here.


================
Comment at: libcxx/include/charconv:501
+
+  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");
----------------
philnik wrote:
> Why _Uglify it? You can just use `type`. Otherwise I would use `_Type` to make it clear that it's a type.
While it's true that `type` is technically not a name that users can macro-ize (otherwise the world would break), it's still good to uglify our internal names for consistency. I don't care strongly about `__type` or `_Type`, but I do have a small preference for `_Type`. What I care strongly about is that we do uglify the name.

The only places were we use `type` should be where the Standard mandates that we have such a user-facing name in our API (in the type traits). I know in practice we sometimes use it for internal-only things because it's convenient, though.


================
Comment at: libcxx/include/type_traits:1024
 
+template <class _Tp>
+using __make_32_64_or_128_bit_t = typename conditional<
----------------
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>;
```


================
Comment at: libcxx/include/type_traits:1025
+template <class _Tp>
+using __make_32_64_or_128_bit_t = typename conditional<
+    is_signed<_Tp>::value,
----------------
philnik wrote:
> Mordante wrote:
> > Note I'm not fond of this name, so I'm open to suggestions for a better name.
> Maybe `__extend_to_32_64_or_128_bit_t`? It's also not exactly perfect, but I think it describes it a bit better.
Just throwing another suggestion out there, but perhaps something like `__widen_up_to_128_t` would be reasonable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128215