Page MenuHomePhabricator

[libc++][format] Add __format_arg_store.

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



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

There are a very large number of changes, so older changes are hidden. Show Older Changes
Mordante marked 9 inline comments as done.Jul 13 2021, 11:29 AM

Addresses most review comments
Uses new XFAIL regex for apple platforms
Minor cleanups

Mordante planned changes to this revision.Jul 13 2021, 11:31 AM

Thanks for the review. I'll some more time to look at your last suggestion.


That's the TODO below. I haven't added proper pointer formatting yet. (In hindsight I should also have removed the nullptr constructor for now.


It's for testing purposes. However when I originally added this I hadn't implemented the visitor yet. So I can used the visitor in the tests and remove this altogether.


Adding some declarations for types that need to know the other types exist.
(And slightly abused to declare some parts of the not yet implemented iterator library. That will be removed once libc++ implements these parts of the C++20 iterator library.)


I did some investigation, but changing this to a built-in array was not trivial. Especially since sizeof...(_Args) can be zero. I still need to implement the required size optimizations for basic_format_args. So I prefer to tackle these two at the same time.
For now I added a TODO in the code.


I think that's a valid point. Most of this test is "identical" to the test libcxx/test/std/utilities/format/format.arguments/format.args/ctor.pass.cpp. For now I changed the test to use libc++ specific asserts.

@ldionne What's your opinion of testing exposition only classes? I think the tests were great to have during development, but I agree with @vitaut that it would be better to test this class indirectly by using the public interface.


For some place where it's easy I already test for char*_t support. I think it would make more sense to restrict that in the formatters. There the Standard is explicit about the supported types. I'll add a note to my todo list to update the formatters.


I really like this suggestion!

Mordante updated this revision to Diff 358384.Jul 13 2021, 12:10 PM

Now really rebase.

Mordante planned changes to this revision.Jul 13 2021, 12:11 PM
Mordante added inline comments.Jul 13 2021, 1:30 PM

I thought MSVC was heavily based on libfmt.

I did some investigations and your suggestion seems to work.
When I make the constructor private I'll need a "factory" or a friend for the internal parts of <format>.
So I'll go for a "factory" so the unit tests and internal implementation can share the same function.

Mordante updated this revision to Diff 358654.Jul 14 2021, 10:03 AM

Make basic_format_context's constructor private and add a helper for its users.
Mark unit tests using the helper as libc++ specific.
Disabled a failing unit tests that fails on GCC.
Make the module headers private.

Mordante marked an inline comment as done.Jul 14 2021, 10:05 AM
Mordante updated this revision to Diff 358682.Jul 14 2021, 11:46 AM

Fixes localization build.
Rebased, hopefully fixes Apple builds.

Mordante updated this revision to Diff 358695.Jul 14 2021, 12:19 PM

Rebase to fix the GCC build.

vitaut accepted this revision.Jul 17 2021, 6:18 AM


vitaut added inline comments.Jul 17 2021, 6:28 AM

I thought MSVC was heavily based on libfmt.

It was loosely based on {fmt} with important pieces removed for some reason (but partly reintroduced now via PRs and issues).

Mordante updated this revision to Diff 359638.Jul 18 2021, 11:07 AM

Guard unit tests for implementation details

Mordante added inline comments.Jul 20 2021, 10:13 AM

s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/ here and all other occurrences in this patch.

Quuxplusone added inline comments.

Re your comment on D106567 — It would be cleaner here to write "one function declaration per function signature," i.e. get rid of the defaulted function argument ( ), and then I bet you wouldn't run into any modules issues with the 2-argument overload (because it wouldn't mention optional in its signature) nor the 3-argument overload (because it wouldn't mention optional in its signature, either). The next step would be to give the two things not-the-same name.
I actually talk about the "ooh, I'll take an optional that defaults to nullopt!" antipattern in Back to Basics: Algebraic Data Types (CppCon 2020), and obviously I've talked about When to Give Two Things the Same Name (CppCon 2021) as well. :)

Mordante updated this revision to Diff 361302.Jul 23 2021, 12:28 PM

etarget for LLVM 14.
Guard better against ADL
Move internal unit tests
Generate private module tests

Mordante updated this revision to Diff 361307.Jul 23 2021, 12:46 PM

Update the private module tests.
These failed due to local merge conflict files.

Mordante marked 3 inline comments as done.Jul 25 2021, 7:10 AM
Mordante added inline comments.

I prefer the current way. This function used to be a constructor before the class' constructor was made private. This was done to prevent users from directly creating basic_format_context objects. I read your blog post before and I partly agree with it, however default arguments are a tool. In this case I feel the code looks better with a default argument instead of having two functions.

I agree this would be bad when the function would take several bool arguments with a defaulted value. IMO a function with multiple bool arguments often is already a code smell by itself.

ldionne requested changes to this revision.Jul 28 2021, 11:02 AM
ldionne added inline comments.

I'm not sure this is worth mentioning at all, since we're going to implement and users will never notice. Also, it'll have been implemented by the time we ship <format> for the first time.


We can remove this now. LLVM 13 has branched, and the oldest compiler we support should support concepts unless I'm mistaken. There will be a similar cleanup in the <ranges> code we merged already.

Note: I need to validate that the oldest compiler we support has support for concepts - I think that would be AppleClang 12, but I'm not sure all versions of it support concepts.


Why aren't we using a variant here?

@vitaut I'm being told you originally requested that Mark moves from variant to this union, was there a reason? I'd like to avoid going back and forth on this issue.


Suggestion to reduce the number of overloads:

template <class _Tp>
  requires (__libcpp_signed_integer<_Tp> || __libcpp_unsigned_integer<_Tp>) // might be a better way to do this
_LIBCPP_HIDE_FROM_ABI explicit basic_format_arg(_Tp __v) noexcept
  if constexpr (sizeof(_Tp) <= sizeof(int)) { }
  else if constexpr (...) { }
  // etc...

Basically, this single constructor should handle all the branches in


IMO I'd rather have a longer line than a funky line break here.

Suggestion: Put _LIBCPP_HIDE_FROM_ABI on a line above, like so:

explicit basic_format_arg(...);

This is what we do in most places. As we just talked about, if this creates pain because of clang-format and rebasing, don't waste your time on this. It's just a suggestion.


Is this [[nodiscard]] really adding anything? Non blocking.

IMO, using nodiscard everywhere because C++ has the wrong default here just adds syntactic overhead and reduces the impact of seeing [[nodiscard]] on a method that could truly be used incorrectly. I think we should stick to putting it on key methods where we really want to diagnose if the user discards the result, because that would most likely be a bug (e.g. anything called empty, something like lock_guard, etc).


Maybe those constructors should all be explicit?


This has been implemented now.


This is not required. We're not shipping the library until it's stable anyway, with the mechanism you added.


Would it be possible to move those tests into libcxx/test/std if we added a function that creates a basic_format_context to the test suite? Different implementations could hook into that function (in the test suite) to tell the test suite how to create a basic_format_context. Do you think that would make sense and be feasible?

I'm trying to see how we can test those parts of the API which are kind of implementation details of the library, but at the same time they're defined in the spec.


Please drop [[nodiscard]] in the tests unless it adds a clear benefit (e.g. a method that could meaningfully be misused).

This revision now requires changes to proceed.Jul 28 2021, 11:02 AM
vitaut added inline comments.Jul 28 2021, 11:36 AM

Not using variant allows optimizing the representation for the common case of a small number of formatting arguments:

Implementations should optimize the representation of basic_­format_­args for a small number of formatting arguments.
[Note 1: For example, by storing indices of type alternatives separately from values and packing the former. — end note]

Quuxplusone added inline comments.Jul 28 2021, 1:36 PM

This open-coded __arg_t __type_ member takes only 1 byte; libc++'s <variant> would use a field __variant_index_t __index_ which takes 4 bytes unless _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION is set.
See D40210.
The Right Answer here might depend on how close we think we are to "ABI v2 by default". Currently that looks like "never". ;)

However however, my understanding of @vitaut's comment is that this code still is not implementing the recommended practice. The recommended practice is that basic_format_args shouldn't store an array of basic_format_arg {union, char} structs at all; it should store a struct of arrays (one array of unions and one array of chars).

I don't think there's any motivation to optimize the layout of basic_format_arg singular. Using variant would be fine, except that it would introduce an extra dependency on <variant> and probably harm compile times.


This is the array-of-structs that @vitaut pointed to. It should be rewritten as a struct-of-arrays.

vitaut added inline comments.Jul 28 2021, 1:52 PM

My understanding is that the optimization is planned for a follow-up diff in which case switch to variant would have to be undone. cc @Mordante

Mordante marked 15 inline comments as done.Jul 29 2021, 7:00 AM
Mordante added inline comments.

This was based on the original plan to have an incomplete version in LLVM 13. But after our discussion last week I agree this is no longer useful. I'll remove the entire remark.


I just tested with a quick hack (D107066) and it seems not all CI nodes support concepts.


Yes the goal is to first have a working implementation where the observable behaviour matches the Standard.
Then this working implementation can be improved. This makes it a lot easier to detect regressions and measure the improvements.
There is a TODO FMT so I won't forget about it.


I experimented with this approach and I wasn't happy with the result. If feels odd to first require __libcpp_signed_integer || __libcpp_unsigned_integer and then in the code test which of the two has been used. Adding char_type and bool made to code even less readable. Especially when char_type is wchar_t it accepts char, but it doesn't allow narrowing the other way around.
So I reduced this set to 4 constructors

  • bool unmodified
  • char_type unmodified
  • __libcpp_signed_integer all in one overload and then use the if constexpr(sizeof(_Tp)...
  • __libcpp_unsigned_integer like above

That IMO leads to a better balance between removing overloads and having readable code.


I tested it. Making this change manually will be reverted by clang-format.


It's very feasible I just need 2 wrapper functions that call std::__format_context_create; one with a std::locale and one without that argument not; basically the same as the std::format* functions.

Whether it makes sense, that depends on whether or not other vendors will supply patches for their hooks.

Mordante updated this revision to Diff 362782.Jul 29 2021, 7:55 AM
Mordante marked 6 inline comments as done.

Rebased and updated to work against main.
Addresses review comments.

ldionne accepted this revision.Aug 31 2021, 11:31 AM
This revision is now accepted and ready to land.Aug 31 2021, 11:31 AM

I read the comments and while I am not a big fan of reimplementing std::variant, I understand why it's needed (unless we want to break std::variant's ABI, which we don't). Also, this needs to make progress so I'm approving it -- worst case I can request some post-commit changes.

Mordante updated this revision to Diff 369930.Sep 1 2021, 8:04 AM

Rebased to give it a CI run test all still is fine.
Remove tests for gcc-10 since it's no longer supported.

Mordante updated this revision to Diff 369931.Sep 1 2021, 8:08 AM

Retry to start CI.

I read the comments and while I am not a big fan of reimplementing std::variant, I understand why it's needed (unless we want to break std::variant's ABI, which we don't). Also, this needs to make progress so I'm approving it -- worst case I can request some post-commit changes.


A note on the std::variant; breaking the ABI won't help. The tags and unions should be stored separately, something like:

std::array<tag, 4> tags;
std::array<unions, 4> unions;

And the note mentions packing the unions, so something like:

std::array<tag, 4> tags;
std::tuple<t0, t1, t2, t3> packed_unions;

(This is an example, I haven't investigated the solution I want to use.)

Mordante updated this revision to Diff 369950.Sep 1 2021, 9:06 AM

Seems Buildkite became unstuck, retry run the CI.

This revision was automatically updated to reflect the committed changes.