[libcxx-commits] [PATCH] D145590: [libc++][charconv] Granularizes the header.

Wed Mar 8 13:35:38 PST 2023

philnik added inline comments.

================
Comment at: libcxx/include/__charconv/to_chars_integral.h:317-334
+template <typename _Tp, typename enable_if<is_integral<_Tp>::value, int>::type = 0>
+inline _LIBCPP_CONSTEXPR_SINCE_CXX23 _LIBCPP_HIDE_FROM_ABI to_chars_result
+to_chars(char* __first, char* __last, _Tp __value)
+{
+  using _Type = __make_32_64_or_128_bit_t<_Tp>;
+  static_assert(!is_same<_Type, void>::value, "unsupported integral type used in to_chars");
+  return std::__to_chars_itoa(__first, __last, static_cast<_Type>(__value), is_signed<_Tp>());
----------------
Mordante wrote:
> philnik wrote:
> > Mordante wrote:
> > > philnik wrote:
> > > > I think we should put these and the floating point overloads in a `__fwd/to_chars.h` header to avoid having a different overload set depending on what file is included. A linker error is a lot better than a program that compiler but is subtly wrong.
> > > I did this on purpose, when I use the floating point formatter, I can include that light header. I don't see a huge issue with this approach. This is only used in the library internally.
> > Adding two forward declarations doesn't make this much heavier. My concern is that someone might rely on this being included transitively, and they get unexpected behaviour.
> I tried it, but then Clang (rightfully) complains about duplicated default arguments. It also pulls in some headers for the floating-point case. So for now I keep it as is.
The default arguments shouldn't be a problem. Simply set the defaults in the forward declaration and include the forward-declaration header when you define them.
What headers are pulled in by that? If it's just `__type_traits/enable_if.h` and `__type_traits/is_integral.h`, I don't think that's a real problem. These headers are tiny.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145590