This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Improve integral formatters.
ClosedPublic

Authored by Mordante on Jun 19 2022, 6:03 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG38adfa91a1f3: [libc++][format] Improve integral formatters.
Summary

This changes the implementation of the formatter. Instead of inheriting
from a specialized parser all formatters will use the same generic
parser. This reduces the binary size.

The new parser contains some additional fields only used in the chrono
formatting. Since this doesn't change the size of the parser the fields
are in the generic parser. The parser is designed to fit in 128-bit,
making it cheap to pass by value.

The new format function is a const member function. This isn't required
by the Standard yet, but it will be after LWG-3636 is accepted.
Additionally P2286 adds a formattable concept which requires the member
function to be const qualified in C++23. This paper is likely to be
accepted in the 2022 July plenary.

This is based on D125606. That commit did the groundwork and did similar
changes for the string formatters.

Depends on D125606

Diff Detail

Event Timeline

Mordante created this revision.Jun 19 2022, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 6:03 AM
Mordante requested review of this revision.Jun 19 2022, 6:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 6:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 438406.Jun 20 2022, 8:09 AM

Try restarting CI.

ldionne requested changes to this revision.Jun 20 2022, 12:06 PM
ldionne added a subscriber: ldionne.

This patch seems to be moving around quite a bit of code. Would it be possible to do some of the reorganization before this patch as a NFC to ease reviewing this?

libcxx/include/__format/formatter_bool.h
59
libcxx/include/__format/formatter_integer.h
24
56

Let's call this __format_impl or something like that, to make it clear that it's not part of the public API.

libcxx/include/__format/formatter_integral.h
123

ADL proofing is technically not needed because we call it with char const*, but I would rather be consistent since it's not a member function.

This revision now requires changes to proceed.Jun 20 2022, 12:06 PM
Mordante updated this revision to Diff 439431.Jun 23 2022, 9:20 AM
Mordante marked 4 inline comments as done.

Rebased and addresses review comments.

This patch seems to be moving around quite a bit of code. Would it be possible to do some of the reorganization before this patch as a NFC to ease reviewing this?

So reorganization has been done as you've seen, for the rest is was quite hard. So I left some comments in this patch.

libcxx/include/__format/formatter_integral.h
147

This hunk was at line 82.

265

Line 254 __format_unsigned_integral

ldionne accepted this revision.Jun 28 2022, 8:55 AM
ldionne added inline comments.
libcxx/include/__format/formatter_integer.h
50

Would it make sense to inline __format into this function instead?

libcxx/include/__format/formatter_integral.h
312

ADL

libcxx/include/__format/parser_std_format_spec.h
1856
This revision is now accepted and ready to land.Jun 28 2022, 8:55 AM
Mordante marked 3 inline comments as done.Jun 28 2022, 9:46 AM

Thanks for the review!

Mordante updated this revision to Diff 440672.Jun 28 2022, 9:58 AM

Addresses review comments and attempts to fix CI.

Mordante updated this revision to Diff 440695.Jun 28 2022, 10:50 AM

Fixes no locale build.

This revision was automatically updated to reflect the committed changes.