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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante 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>());
----------------
philnik wrote:
> 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.
I just dislike that solution where we have two declarations.

Instead I added a new `to_chars.h` private header including them both. So you can pick that one or if you know what you're doing the even more specialized ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145590