This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Implements LWG3892.
ClosedPublic

Authored by Mordante on Mar 11 2023, 6:55 AM.

Details

Reviewers
ldionne
vitaut
EricWF
Group Reviewers
Restricted Project
Commits
rG9b43aedeb311: [libc++][format] Implements LWG3892.
Summary

This LWG issue is based on the discussion regarding

P2733R3 Fix handling of empty specifiers in std::format

This paper was disussed and changed a few times in LEWG during the
Issaquah meeting. The paper was not voted in, instead LEWG asked for
a DR against C++26.

This LWG issue contains the direction voted by LEWG. This issue has not
been voted in yet. However it fixes some of the defencies on the
container based formatting. Without this fix the range-default-formatter
for strings looks bad when used in containers.

The changes of this issue match the intended changes of P27333.

type fmt before after (if changed)

char {} a
char {:?} 'a'
array<char, 1> {} ['a']
array<char, 1> {::} [a]
array<char, 1> {::c} [a]
array<char, 1> {::?} ['a']
map<char, char> {} {a: a} -> {'a': 'a'}
map<char, char> {::} {'a': 'a'}
set<char> {} {'a'}
set<char> {::} {a}
set<char> {::c} {a}
set<char> {::?} {'a'}
tuple<char> {} ('a')
stack<char> {} ['a']
stack<char> {::} [a]
stack<char> {::c} [a]
stack<char> {::?} ['a']
array<array<char, 1>, 1> {} [[a]] -> {'a': 'a'}
array<array<char, 1>, 1> {::} [['a']]
array<array<char, 1>, 1> {:::} [[a]]
array<array<char, 1>, 1> {:::c} [[a]]
array<array<char, 1>, 1> {:::?} [['a']]
array<tuple<char>, 1> {} [(a)] -> [('a')]
tuple<tuple<char>> {} ((a)) -> (('a'))
tuple<array<char, 1>> {} ([a]) -> (['a'])

Note the optimization text as mentioned in the tuple formatter can't be
done. The call to parse may affect the formatter so its state needs to
be preserved.

Diff Detail

Event Timeline

Mordante created this revision.Mar 11 2023, 6:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 6:55 AM
Mordante published this revision for review.Mar 12 2023, 7:10 AM
Mordante added reviewers: ldionne, vitaut.
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:10 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision as: ldionne.Mar 14 2023, 8:59 AM

LGTM, it would be great if @vitaut could have a quick look as well.

libcxx/test/support/format.functions.common.h
52–57
EricWF accepted this revision as: EricWF.Mar 17 2023, 2:34 PM
EricWF added a subscriber: EricWF.

Nice tests. Big fan of the work done in this patch.

I haven't reviewed it for correctness w.r.t. the format spec, but in general I think it looks good.

libcxx/include/__format/range_formatter.h
265

Is there a reason for always doing this rather than just guarding it in a macro?

ldionne accepted this revision.Mar 28 2023, 8:59 AM
ldionne added inline comments.
libcxx/test/std/utilities/format/format.range/format.range.fmtmap/format.functions.format.pass.cpp
43

This change seems like a leftover?

This revision is now accepted and ready to land.Mar 28 2023, 8:59 AM
Mordante marked 3 inline comments as done.Apr 8 2023, 2:13 AM

Thanks for the reviews!

libcxx/include/__format/range_formatter.h
265

I'm not entirely sure what you mean. But we need to call __underlying_.parse(__parse_ctx); even when the range is empty.

Calling the parse member of the formatter is an observable effect by the parser, there are tests to validate the call is made.

Mordante updated this revision to Diff 511866.Apr 8 2023, 2:13 AM
Mordante marked an inline comment as done.

Rebased and addresses review comments. Giving it a CI run.

This revision was landed with ongoing or failed builds.Apr 8 2023, 5:12 AM
This revision was automatically updated to reflect the committed changes.