This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Adds a formattable concept.
ClosedPublic

Authored by Mordante on Mar 3 2022, 10:28 AM.

Details

Reviewers
vitaut
ldionne
Mordante
Group Reviewers
Restricted Project
Commits
rG15c809e8e780: [libc++][format] Adds a formattable concept.
Summary

The concept is based on P2286R2 Formatting Ranges. It will be used to
optimise the storage of __format_arg_store as required by LWG-3473.

Depends on D120916

Diff Detail

Event Timeline

Mordante created this revision.Mar 3 2022, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 10:28 AM
Herald added a subscriber: mgorny. · View Herald Transcript
Mordante requested review of this revision.Mar 3 2022, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 10:28 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 412776.Mar 3 2022, 10:56 AM

Remove a part of a test intended for another commit.

Quuxplusone added inline comments.
libcxx/include/__format/concepts.h
39–45

Let's add some linebreaks for readability after each requires-parameter:

template <class _Tp, class _CharT>
concept __formattable =
  semiregular<formatter<remove_cvref_t<_Tp>, _CharT>> &&
  requires(formatter<remove_cvref_t<_Tp>, _CharT> __f,
           formatter<remove_cvref_t<_Tp>, _CharT> __cf, // TODO: const
           _Tp __t,
           basic_format_context<_CharT*, _CharT> __fc,
           basic_format_parse_context<_CharT> __pc) {
    { __f.parse(__pc) } -> same_as<typename basic_format_parse_context<_CharT>::iterator>;
    { __cf.format(__t, __fc) } -> same_as<_CharT*>;
  };

Please ping me on Discord if I should be reviewing this -- my understanding is that it's for a not-yet-approved paper? (if so, you can mark this as "changes planned" until you'd like a review)

libcxx/include/__format/concepts.h
33

This can be rebased out!

Mordante updated this revision to Diff 416689.Mar 19 2022, 5:08 AM

Rebased and address review comments.

Mordante marked 2 inline comments as done.Mar 19 2022, 5:15 AM

Please ping me on Discord if I should be reviewing this -- my understanding is that it's for a not-yet-approved paper? (if so, you can mark this as "changes planned" until you'd like a review)

Correct the paper isn't accepted yet and the paper targets C++23. However as mentioned in the description I need a formattable concept in C++20. I use the concept in D121514. So I either:

  • Write my own concept, which would look slightly different.
  • Use P2286's proposed concept.

Using an existing proposal allows me to reuse this "private" concept in C++23 avoiding code duplication.
So even when P2286 will never make it in the Standard this concept is still useful.
(I hope P2286 will make it in C++23 since I really like to proposal.)

libcxx/include/__format/concepts.h
39–45

I did part of the formatting.

vitaut added inline comments.Mar 27 2022, 7:21 AM
libcxx/include/__format/concepts.h
39

I think you should use format_context::iterator, not _CharT*.

Mordante marked 2 inline comments as done.Mar 27 2022, 9:05 AM

Thanks for the review!

libcxx/include/__format/concepts.h
39

That doesn't works since that only works for the char context. I'll change to code to use fmt-iter-for<charT> as used in the paper. I prefer to use the CharT* since it's the most simple output iterator. This is a valid choice according to the paper.

We don’t specify what the iterator type is of format_context or wformat_context, the expectation is that formatters accept any iterator. As such, it is unspecified in the concept which iterator will be checked - simply that it is some output_iterator<charT const&>. Implementations could use format_context::iterator and wformat_context::iterator, or they could have a bespoke minimal iterator dedicated for concept checking.
Mordante updated this revision to Diff 418457.Mar 27 2022, 9:06 AM
Mordante marked an inline comment as done.

Address review comments.

vitaut added inline comments.Mar 27 2022, 9:09 AM
libcxx/include/__format/concepts.h
39

Makes sense but maybe clarify it in a comment?

Mordante updated this revision to Diff 418458.Mar 27 2022, 9:17 AM

Add comments as suggested.

Mordante marked an inline comment as done.Mar 27 2022, 9:17 AM
Mordante updated this revision to Diff 421703.Apr 9 2022, 12:50 AM

Rebased and updated to new test method.

Mordante updated this revision to Diff 427409.May 5 2022, 11:46 AM

Rebased to trigger CI.

Mordante accepted this revision.May 18 2022, 10:58 AM
This revision is now accepted and ready to land.May 18 2022, 10:58 AM
This revision was automatically updated to reflect the committed changes.