This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Mordante on Mar 8 2023, 8:11 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGf7efcaca77d8: [libc++][charconv] Granularizes the header.
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.

Diff Detail

Event Timeline

Mordante created this revision.Mar 8 2023, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 8:11 AM
Herald added a subscriber: arichardson. · View Herald Transcript
Mordante requested review of this revision.Mar 8 2023, 8:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 8:11 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 503390.Mar 8 2023, 8:15 AM

Retrigger CI.

philnik added a subscriber: philnik.Mar 8 2023, 8:17 AM
philnik added inline comments.
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
690–692

Why do you deviate from the normal style here?

Mordante marked 2 inline comments as done.Mar 8 2023, 8:45 AM
Mordante added inline comments.
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
690–692

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

Mordante updated this revision to Diff 503396.Mar 8 2023, 8:47 AM
Mordante marked 2 inline comments as done.

CI fixes and addresses review comments.

philnik added inline comments.Mar 8 2023, 8:50 AM
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 marked an inline comment as done.Mar 8 2023, 1:29 PM
Mordante added inline comments.
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 updated this revision to Diff 503498.Mar 8 2023, 1:32 PM
Mordante marked an inline comment as done.

CI fixes.

philnik added inline comments.Mar 8 2023, 1:35 PM
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 updated this revision to Diff 503788.Mar 9 2023, 8:28 AM

CI fixes and adresses a review comment.

Mordante marked an inline comment as done.Mar 9 2023, 8:29 AM
Mordante added inline comments.
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 accepted this revision.Mar 14 2023, 8:39 AM
ldionne added a subscriber: 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.

This revision is now accepted and ready to land.Mar 14 2023, 8:40 AM
Mordante marked 2 inline comments as done.Mar 14 2023, 10:12 AM

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 updated this revision to Diff 505159.Mar 14 2023, 10:21 AM

Rebasing gave merge conflicts, give it another CI run.

philnik added inline comments.Mar 14 2023, 10:21 AM
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 updated this revision to Diff 505163.Mar 14 2023, 10:33 AM

Retrigger CI.

Mordante updated this revision to Diff 505230.Mar 14 2023, 1:07 PM

CI Fixes.

This revision was landed with ongoing or failed builds.Mar 15 2023, 10:01 AM
This revision was automatically updated to reflect the committed changes.