This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] range-default-formatter for strings.
ClosedPublic

Authored by Mordante on Mar 11 2023, 10:05 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rGed8ea2bbf843: [libc++][format] range-default-formatter for strings.
Summary

Implements the range-default-formatter specialization range_format::string
and range_format::debug_string.

Implements parts of

  • P2286R8 Formatting Ranges
  • P2585R0 Improving default container formatting

Depends on D145847

Diff Detail

Event Timeline

Mordante created this revision.Mar 11 2023, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2023, 10:05 AM
JMazurkiewicz added inline comments.
libcxx/include/__format/range_default_formatter.h
195–196

I think that the condition should be ranges::contiguous_range<_Rp> && ranges::sized_sentinel_for<ranges::sentinel_t<_Rp>, ranges::iterator_t<_Rp>>, since those are the constraints of basic_string_view's constructor (http://eel.is/c++draft/string.view.template#string.view.cons-7). Also begin() and end() functions are not required to be the members of range:

// https://godbolt.org/z/xzvG5Y36Y
struct X { };
int* begin(X);
int* end(X);
static_assert(ranges::contiguous_range<X>); // OK!

I think that You should try simpler approach that is already used in range_formatter: https://github.com/llvm/llvm-project/blob/43ae4b62b2671cf73e691c0b53324cd39405cd51/libcxx/include/__format/range_formatter.h#L170-L179

Mordante updated this revision to Diff 504430.Mar 12 2023, 6:37 AM

Addresses review comment.

Mordante marked an inline comment as done.Mar 12 2023, 6:40 AM

Thanks for the review!

libcxx/include/__format/range_default_formatter.h
195–196

Thanks, good catch. This patch was written quite a while ago and I forgot to update it with the changes in the range_foramatter. (I didn't upload it before since there were issues when using these formatters in ranges, which is addressed by LWG3892.)

Mordante published this revision for review.Mar 12 2023, 7:10 AM
Mordante added reviewers: ldionne, vitaut.
Mordante marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2023, 7:10 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
h-vetinari added inline comments.
libcxx/docs/Status/FormatPaper.csv
40–42

This line is fully duplicated (both before and after; it got introduced by mistake AFAICT, and should be removed again)

Mordante added inline comments.Mar 17 2023, 9:32 AM
libcxx/include/__format/range_default_formatter.h
175

Remove availability.

Mordante added inline comments.Mar 22 2023, 9:24 AM
libcxx/test/std/utilities/format/format.range/format.range.fmtstr/format.pass.cpp
14

Review all test to match the availablity as changed in D134598.

ldionne accepted this revision.Mar 28 2023, 9:15 AM
ldionne added inline comments.
libcxx/include/__format/range_default_formatter.h
180
libcxx/test/std/utilities/format/format.range/format.range.fmtstr/format.functions.tests.h
30–31
45–46
This revision is now accepted and ready to land.Mar 28 2023, 9:15 AM
Mordante marked 4 inline comments as done.Apr 8 2023, 5:19 AM
Mordante added inline comments.
libcxx/docs/Status/FormatPaper.csv
40–42

Thanks the second should have been debug_string.

Mordante updated this revision to Diff 511881.Apr 8 2023, 5:19 AM
Mordante marked an inline comment as done.

Rebased and addresses review comments, give a CI run before landing.

Mordante updated this revision to Diff 511894.Apr 8 2023, 8:49 AM

CI fixes.

Mordante updated this revision to Diff 511906.Apr 8 2023, 10:30 AM

CI fixes.

This revision was landed with ongoing or failed builds.Apr 9 2023, 3:48 AM
This revision was automatically updated to reflect the committed changes.