This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Improve floating-point formatters.
ClosedPublic

Authored by Mordante on Jun 28 2022, 11:40 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG152d92229580: [libc++][format] Improve floating-point 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.

Diff Detail

Event Timeline

Mordante created this revision.Jun 28 2022, 11:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2022, 11:40 PM
Mordante added inline comments.Jun 29 2022, 10:29 AM
libcxx/include/__format/formatter_floating_point.h
430–489

Moved from line 642.

576

Moved from line 609.

libcxx/include/__format/formatter_output.h
225

Copied from __format/formatter.h:194

Mordante updated this revision to Diff 441077.Jun 29 2022, 10:42 AM

Fixes CI and fixes some potential ADL issues.

Mordante published this revision for review.Jun 29 2022, 12:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 12:30 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jul 6 2022, 8:48 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/__format/formatter_floating_point.h
586–591

I know this was simply moved, however I think we should at least refactor the uppercase determination into a separate variable like this:

bool __uppercase = __specs.__std_.__type_ == __format_spec::__type::__hexfloat_upper_case ||
                  __specs.__std_.__type_ == __format_spec::__type::__scientific_upper_case ||
                  __specs.__std_.__type_ == __format_spec::__type::__fixed_upper_case ||
                  __specs.__std_.__type_ == __format_spec::__type::__general_upper_case;

I'm not a big fan of the way this is written because it's a bit too clever, but I can't think of an easy way to keep it branchless with another implementation, so that's fine.

This revision is now accepted and ready to land.Jul 6 2022, 8:48 AM
Mordante marked an inline comment as done.Jul 6 2022, 10:04 AM

Thanks for the review!

Mordante updated this revision to Diff 442622.Jul 6 2022, 10:12 AM

Rebased and addresses the review comment.

This revision was automatically updated to reflect the committed changes.
libcxx/include/__format/parser_std_format_spec.h