This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Implements range_formatter
ClosedPublic

Authored by Mordante on Dec 24 2022, 6:01 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rG22e8525dfdd7: [libc++][format] Implements range_formatter
Summary

Implements parts of

  • P2286R8 Formatting Ranges
  • P2585R0 Improving default container formatting

Depends on D140651

Diff Detail

Event Timeline

Mordante created this revision.Dec 24 2022, 6:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 24 2022, 6:01 AM
Mordante updated this revision to Diff 485203.Dec 24 2022, 9:03 AM

CI fixes.

Mordante updated this revision to Diff 485204.Dec 24 2022, 9:46 AM

CI fixes.

Mordante updated this revision to Diff 485379.Dec 27 2022, 8:01 AM

CI fixes and polishing.

Mordante updated this revision to Diff 485397.Dec 27 2022, 10:30 AM

CI fixes and polishing.

Mordante published this revision for review.Dec 28 2022, 9:22 AM
Mordante added reviewers: ldionne, vitaut.
Mordante added inline comments.
libcxx/include/__format/range_formatter.h
110

I will look at better error message in the future. I think it makes sense to review them all and make sure they get a consistent style.

Herald added a project: Restricted Project. · View Herald TranscriptDec 28 2022, 9:22 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Jan 10 2023, 9:42 AM
libcxx/include/__format/range_formatter.h
165

Here and elsewhere.

189

Can you see a way to de-duplicate this function and __format_as_string by e.g. passing the __specs into it?

224

I think you're assuming that string::iterator is a char* here.

libcxx/test/std/utilities/format/format.range/format.range.formatter/underlaying.pass.cpp
1 ↗(On Diff #485397)

Typo in the filename

Mordante updated this revision to Diff 488275.Jan 11 2023, 10:16 AM
Mordante marked 3 inline comments as done.

Rebased and addresses review comments.

ldionne requested changes to this revision.Jan 17 2023, 9:06 AM
ldionne added inline comments.
libcxx/include/__format/buffer.h
498

I wouldn't guard this as being >= C++23, since this is an implementation detail. I would only guard the range formatter itself.

Same applies below.

514–515

Would it be possible to use a std::vector internally instead of implementing this manually?

libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.format.pass.cpp
1

Since this is a general range formatter, I would like to see tests for formatting different kinds of ranges, like an input range, forward range, etc.

libcxx/test/std/utilities/format/format.range/format.range.formatter/set_brackets.pass.cpp
39

Let's keep the comment, but we can still add a simple test to validate that we're using the right brackets when we output a simple range.

libcxx/test/std/utilities/format/format.range/format.range.formatter/set_separator.pass.cpp
38

Same comment as for brackets.

This revision now requires changes to proceed.Jan 17 2023, 9:06 AM
Mordante marked 6 inline comments as done.Jan 17 2023, 11:35 AM

Thanks for the review!

libcxx/include/__format/buffer.h
514–515

Yes, that's possible. It might have a small speed penalty, but since its usage is rare I think it's not too bad.

libcxx/include/__format/range_formatter.h
224

I assume basic_string_view<_CharT>::const_iterator is _CharT*. Which is currently true for libc++. I used to be convinced this was required by the Standard, but I recently learned this is not true.

I agree this should be fixed, but I prefer to do that in a different commit. In that commit I should fix all these assumptions. (All parsing code in format makes this assumption.)

I even wonder whether we should change basic_string_view to use a wrap_iterator.

libcxx/test/std/utilities/format/format.range/format.range.formatter/set_brackets.pass.cpp
39

I added a simple format test. Note set_brackets is constexpr, but format isn't (yet). So the additional validation is only done in the non-constexpr case.

Mordante updated this revision to Diff 489902.Jan 17 2023, 11:45 AM
Mordante marked 3 inline comments as done.

Addresses review comments.

Mordante updated this revision to Diff 489922.Jan 17 2023, 12:48 PM

Retest CI.

Mordante updated this revision to Diff 490170.Jan 18 2023, 8:02 AM

Rebased to test CI.

Mordante updated this revision to Diff 490182.Jan 18 2023, 8:33 AM

Disable more tests in GCC.

ldionne accepted this revision.Jan 18 2023, 12:02 PM
This revision is now accepted and ready to land.Jan 18 2023, 12:02 PM
This revision was automatically updated to reflect the committed changes.

Hey, it looks like this change (and maybe https://reviews.llvm.org/D141290) is breaking the Fuchsia toolchain builders: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-arm64/b8791522961721981361/test-results?sortby=&groupby=

It seems likely that we don't have the fr_FR locale generated on our bots, so I wouldn't expect these tests to work. Is there a way to feature flag these off so that the corresponding portions are disabled if the locale is unavailable?

Hey, it looks like this change (and maybe https://reviews.llvm.org/D141290) is breaking the Fuchsia toolchain builders: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-arm64/b8791522961721981361/test-results?sortby=&groupby=

It seems likely that we don't have the fr_FR locale generated on our bots, so I wouldn't expect these tests to work. Is there a way to feature flag these off so that the corresponding portions are disabled if the locale is unavailable?

Sorry for the inconvenience. Unfortunately it's not possible to selectively disable that part of the test. For now I've disabled it unconditionally.
https://reviews.llvm.org/rGd819703410c2362cbafb60117dab45b182d8b13d

I will look at a proper solution, but I would like to have that reviewed properly.

It seems currently the fuchsia buildbot has unrelated issues. Can you test whether main works again with my patch?

It seems currently the fuchsia buildbot has unrelated issues. Can you test whether main works again with my patch?

The builders are green now; thanks for your help!

It seems currently the fuchsia buildbot has unrelated issues. Can you test whether main works again with my patch?

The builders are green now; thanks for your help!

Thanks for the update!