[libc++][charconv] Granularizes the header.

[Mordante]

Summary

Having the header granularized makes it possible to remove the
dependency on this header in <format>. This <format> header gets
included in more headers due to more usage of std::formatter in the
library. This should reduce the number of transitive includes.

Note formatting the new headers will be done in a followup patch.

[Mordante]

Retrigger CI.

[philnik]

libcxx/include/__charconv/to_chars_integral.h

318–335

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.

libcxx/include/__charconv/traits.h

2

FWIW I'd be fine with formatting the files in this patch.

libcxx/include/module.modulemap.in

687–689

Why do you deviate from the normal style here?

[Mordante]

libcxx/include/__charconv/to_chars_integral.h

318–335

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.

libcxx/include/__charconv/traits.h

2

I really prefer to do it separately. I also want to format the other headers in this directory.
(I will only make a draft of it to test the CI and land it so no additional reviewing is required.)

libcxx/include/module.modulemap.in

687–689

Good point, I should increment the indention of the others.

[Mordante]

CI fixes and addresses review comments.

[philnik]

libcxx/include/__charconv/to_chars_integral.h

318–335

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.

[Mordante]

libcxx/include/__charconv/to_chars_integral.h

318–335

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.

[Mordante]

CI fixes.

[philnik]

libcxx/include/__charconv/to_chars_integral.h

318–335

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.

[Mordante]

CI fixes and adresses a review comment.

[Mordante]

libcxx/include/__charconv/to_chars_integral.h

318–335

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.

[ldionne]

LGTM -- I am fine with either splitting the floating point to_chars and the integral ones or not splitting them. I think the current approach with a combined header that includes the floating-point/integral headers is a fine compromise.

[Mordante]

In D145590#4193285, @ldionne wrote:

LGTM -- I am fine with either splitting the floating point to_chars and the integral ones or not splitting them. I think the current approach with a combined header that includes the floating-point/integral headers is a fine compromise.

Thanks for the review!

[Mordante]

Rebasing gave merge conflicts, give it another CI run.

[philnik]

libcxx/include/__charconv/to_chars_integral.h

318–335

I really don't get why you are so vehemently against this. It's really not a lot of work and could easily fix super subtle bugs. I also don't think that adding another header is a fix for this. It still splits up the interface.

[Mordante]

Retrigger CI.

[Mordante]

CI Fixes.