Page MenuHomePhabricator

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

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


Group Reviewers
Restricted Project

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


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

Note 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
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

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".


Do we have to include all of functional for invoke?


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


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


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.


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


create -> created


allow -> allows


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


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


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


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.


It seems we do.


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.


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


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.


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


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


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.