Page MenuHomePhabricator

[libc++][format] Add __format_arg_store.
Needs ReviewPublic

Authored by Mordante on Sat, May 29, 1:29 AM.

Details

Reviewers
curdeius
ldionne
miscco
vitaut
Group Reviewers
Restricted Project
Summary

This implements the struct __format_arg_store and its dependencies:

  • the class basic_format_arg,
  • the class basic_format_args,
  • the class basic_format_context,
  • the function make_format_args,
  • the function wmake_format_args,
  • the function visit_format_arg,
  • several Standard required typedefs.

The following parts will be implemented in a later patch:

  • the child class basic_format_arg::handle,
  • the function basic_format_arg::basic_format_arg(const T* p).

The following extension has been implemented:

  • the class basic_format_arg supports __[u]int128_t on platform where libc++ supports 128 bit integrals.

Implements parts of:

  • P0645 Text Formatting

Completes:

  • LWG3371 visit_format_arg and make_format_args are not hidden friends
  • LWG3542 basic_format_arg mishandles basic_string_view with custom traits

Note https://mordante.github.io/blog/2021/06/05/format.html gives a bit more information about the goals and non-goals of this initial patch series.

Diff Detail

Event Timeline

Mordante created this revision.Sat, May 29, 1:29 AM
Mordante requested review of this revision.Sat, May 29, 1:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, May 29, 1:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante updated this revision to Diff 348623.Sat, May 29, 1:41 AM

Remove non-ASCII comment.

Mordante updated this revision to Diff 348627.Sat, May 29, 4:15 AM

Fixes build for platforms without 128-bit support.

Mordante updated this revision to Diff 349040.Tue, Jun 1, 11:42 AM

Rebased on main
Polishing
implement a real output_iterator concept, main has the require prerequisite.

Mordante updated this revision to Diff 349056.Tue, Jun 1, 12:20 PM

Attempt to fix the build.

Mordante added inline comments.Tue, Jun 1, 12:48 PM
libcxx/include/module.modulemap
614

Work-around. Needs to be removed when the modules work properly in main.

Mordante edited the summary of this revision. (Show Details)Sat, Jun 5, 3:59 AM
Mordante added reviewers: curdeius, ldionne, miscco, vitaut.
Mordante edited the summary of this revision. (Show Details)Tue, Jun 8, 10:50 PM
Mordante updated this revision to Diff 350782.Tue, Jun 8, 10:59 PM

Fixes an bug in the format_arg constructor for string_view.
Improve the format_arg constructor for string by addressing LWG3542.

Note the paper status will be updated once the C++2b LWG issues have been updated.

vitaut requested changes to this revision.Tue, Jun 15, 7:30 AM

"the function make_format_args," appears twice in the summary. I guess the second one was supposed to be "make_wformat_args".

libcxx/include/__format/format_arg.h
18

Do we have to include all of functional for invoke?

70

Visiting an empty argument should pass monostate to the visitor, not throw an exception.

101

Why store a C string as void* and cast? I suggest storing as const char_type* instead.

117

Initializing __bool is unnecessary because the value is never accessed if type is __none. Probably doesn't matters in practice but there is no harm is doing less unnecessary work.

libcxx/include/__format/format_context.h
52–63

basic_format_context is not supposed to be constructed by users so these ctors better be private.

93

create -> created

94

allow -> allows

95–96

Lazy creation is fine but using optional is suboptimal since locale is basically a pointer.

libcxx/include/format
18

It is no longer considering, the paper has been accepted.

240–244

This is identical to make_format_args so it just adds unnecessary indirection and a ton of template instantiation. I suggest removing.

libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp
29

This better be parameterized on char type, not context (and the same throughout the tests).

This revision now requires changes to proceed.Tue, Jun 15, 7:30 AM
Mordante planned changes to this revision.Wed, Jun 16, 11:31 AM
Mordante marked 11 inline comments as done and an inline comment as not done.

Thanks for the review!

"the function make_format_args," appears twice in the summary. I guess the second one was supposed to be "make_wformat_args".

Good catch.

libcxx/include/__format/format_arg.h
18

It seems we do.

70

I thought this was an allowed optimization, but that seems not to be the case.
Adding monostate will require including <variant>. At the moment we're busy modularizing our headers, but that unveils some clang issues. So I'll include the entire header with a TODO.
This change will break some other patches; I'll update them later.

117

I recall I originally had it like that, but that caused some issues. However testing again it seems not to be required (anymore).

libcxx/include/__format/format_context.h
52–63

Not being able construct it makes it hard to test the implementation. So I prefer to keep them public.
MSVC STL also implements their constructors as public.

95–96

True, want to look at a better solution later, hence the TODO. I initially wanted to focus on getting something working and optimize later.

libcxx/include/format
18

:-) (It was valid at the time of writing, I probably need to update the status of P2216 at more places.)

libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp
29

Nice suggestion, that makes the callers a bit less verbose.

Mordante edited the summary of this revision. (Show Details)Wed, Jun 16, 11:31 AM
Mordante updated this revision to Diff 352779.Thu, Jun 17, 10:11 AM
Mordante marked 7 inline comments as done.

Addresses review comments.
Update module map.
Update P2216 status; it will be part of C++23 and backported to C++20.

Mordante updated this revision to Diff 352788.Thu, Jun 17, 10:50 AM

Tabs -> space.