This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] Move format_to_n_result.
ClosedPublic

Authored by Mordante on Nov 13 2021, 10:47 AM.

Details

Reviewers
Quuxplusone
Mordante
Group Reviewers
Restricted Project
Commits
rG5baa4ee30b5c: [libc++][NFC] Move format_to_n_result.
Summary

Places format_to_n_result to its own file. While working on D112361 it
turns out the type will be used outside the format header.

Diff Detail

Event Timeline

Mordante created this revision.Nov 13 2021, 10:47 AM
Mordante requested review of this revision.Nov 13 2021, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 13 2021, 10:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 387039.Nov 13 2021, 11:17 AM

Fixes building on Apple. (Basically disabling the build.)

Mordante updated this revision to Diff 387043.Nov 13 2021, 12:02 PM

Fixes modular build.

Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/include/__format/format_to_n_result.h
25–28

FWIW, I think this TODO is obvious. I'm not worried that we'll forget to eliminate _LIBCPP_HAS_NO_CONCEPTS once that becomes possible. :)

libcxx/include/format
581

LGTM, ship it (once CI passes).
But FWIW, for any header where we're doing the "granular" thing, I would strongly prefer that the top-level header contain nothing but #include directives (and boilerplate). That is, I think you should follow up, as soon as you get the cycles, by moving format, formatted_size, etc., into their own headers.

I also don't understand why you're splitting up struct format_to_n_result from the format_to_n function itself; shouldn't they go together into <__format/format_to_n.h>?

Mordante marked 2 inline comments as done.Nov 14 2021, 6:11 AM

Thanks for the review!

libcxx/include/__format/format_to_n_result.h
25–28

True but this is the comment I've used before and try to keep it consistent ;-)
I would really love to get rid of this TODO...

libcxx/include/format
581

I'm not going to mvoe the format functions for now. I'm happy to add a TODO FMT for now. The reason not to do it now it to avoid merge conflicts with the P2216 patches under review now. (I've some more patches not yet under review, so I strongly prefer to wait with this until it won't cause needless rebase issues.)

I moved only the struct to a new header since it will be used in the __format/buffer.h header and in format. This is part of the PoC P2216 patch D112361. Currently I'm reworking that patch to patches in that stack to have small modular patches.

Mordante accepted this revision.Nov 16 2021, 6:48 AM
Mordante marked 2 inline comments as done.
This revision is now accepted and ready to land.Nov 16 2021, 6:48 AM
This revision was automatically updated to reflect the committed changes.