This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Adds range-default-formatter.
ClosedPublic

Authored by Mordante on Nov 2 2022, 10:17 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rGd184958bad5c: [libc++][format] Adds range-default-formatter.
Summary

This adds an incomplete version where the specializations for the
format_kinds are disabled dummy formatters.

Implements part of

  • P2585R0 Improving default container formatting

Diff Detail

Event Timeline

Mordante created this revision.Nov 2 2022, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 10:17 AM
Mordante updated this revision to Diff 472681.Nov 2 2022, 10:30 AM

Remove copy pasted non-ASCII

Mordante updated this revision to Diff 472760.Nov 2 2022, 2:13 PM

Try to fix CI.

Mordante updated this revision to Diff 473423.Nov 5 2022, 5:54 AM

CI fixes.

Mordante updated this revision to Diff 473449.Nov 5 2022, 11:49 AM

CI fixes.

Mordante updated this revision to Diff 473465.Nov 5 2022, 1:57 PM

Rebased and CI fix

Mordante updated this revision to Diff 473506.Nov 6 2022, 11:37 AM

Rebase and CI fixes.

Mordante updated this revision to Diff 473683.Nov 7 2022, 8:03 AM

CI fixes.

Mordante updated this revision to Diff 473733.Nov 7 2022, 10:11 AM

Attempt to fix CI

Mordante updated this revision to Diff 473752.Nov 7 2022, 11:36 AM

Try to fix CI.

Mordante updated this revision to Diff 478262.Nov 28 2022, 8:53 AM

Rebased and add a link to an LWG issue.

Mordante updated this revision to Diff 478270.Nov 28 2022, 9:33 AM

CI fixes.

Mordante published this revision for review.Nov 29 2022, 9:47 AM
Mordante added reviewers: ldionne, vitaut.
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 9:47 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
vitaut added inline comments.Dec 3 2022, 8:39 AM
libcxx/include/__format/concepts.h
63–66

We definitely don't want to use the concept from P2165 because it includes array (maybe clarify this in the comment). The naming they took is very unfortunate but at least it's exposition-only.

68–70

This is actually not tuple-like but pair-like so I suggest renaming to __fmt_pair_like.

libcxx/include/__format/range_default_formatter.h
39–40

What does this shadow?

57

nit: __get_format_format looks a bit repetitive, maybe rename to __get_range_format (since it only applies to ranges) or something else?

libcxx/include/__type_traits/is_specialization.h
35–39

This is not related to formatting and should probably be split into a separate diff.

Mordante marked 4 inline comments as done.Dec 4 2022, 3:31 AM

Thanks for the review!

libcxx/include/__format/concepts.h
63–66

Correct, I added comment to avoid possible confusion between the two concepts.

68–70

Good point!

libcxx/include/__format/range_default_formatter.h
39–40

set, map and string. Not all of them now, but they will once the other specializations are implemented. Since it raises questions I've added a comment.

57

Good catch, it should have been __get_range_format in the first place.

libcxx/include/__type_traits/is_specialization.h
35–39

I will do that before committing. Adding the trait without a use-case also looks odd.

Mordante updated this revision to Diff 479909.Dec 4 2022, 3:33 AM
Mordante marked 4 inline comments as done.

Addresses review comments.

ldionne accepted this revision.Dec 6 2022, 9:23 AM

Looks pretty good to me, just a few comments.

libcxx/include/__format/concepts.h
63

Thanks for adding this comment! I would suggest linking to the section of the standard that defines that concept instead of the paper.

68–70

Can you make sure to have a test that would fail if we were using the concept from P2165?

libcxx/include/__format/range_default_formatter.h
55–56

If that works, that would be a bit nicer and it would remove the need for the additional type + comment.

65

Can you ensure that you have a test with a range where the reference type is the same as the range, without depending on filesystem? You should also keep your current test for filesystem::path.

84

FWIW I wouldn't mind if you used an immediately-invoked lambda expression here. Not a request, just a suggestion.

86–93

After consideration, I'm fine with this. I'd much rather remove the offending formatter but let's wait for the LWG issue resolution before we do that.

99
libcxx/test/libcxx/type_traits/is_specialization.compile.pass.cpp
46
This revision is now accepted and ready to land.Dec 6 2022, 9:23 AM
Mordante marked 7 inline comments as done.Dec 6 2022, 11:22 AM

Thanks for the review!

libcxx/include/__format/concepts.h
68–70

As discussed I added a TODO for that.

libcxx/include/__format/range_default_formatter.h
55–56

This works. Thanks for the suggestion!
(I changed the type back to range_format to avoid another error.)

65

I added a test to libcxx/test/std/utilities/format/format.range/format.range.fmtkind/format_kind.compile.pass.cpp.

libcxx/include/__type_traits/is_specialization.h
35–39

I discussed with @ldionne and he likes to do it in the same commit.

Mordante updated this revision to Diff 480555.Dec 6 2022, 11:25 AM
Mordante marked 3 inline comments as done.

Rebased and address review comments.
Make sure the CI still passes.

Mordante updated this revision to Diff 480564.Dec 6 2022, 11:35 AM

CI fixes.

This revision was landed with ongoing or failed builds.Dec 7 2022, 8:33 AM
This revision was automatically updated to reflect the committed changes.