Page MenuHomePhabricator

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

Authored by Mordante on May 29 2021, 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

There are a very large number of changes, so older changes are hidden. Show Older Changes

Tabs -> space.

vitaut accepted this revision.Jun 20 2021, 6:44 AM

LGTM but this will need to be reviewed by someone familiar with libc++ of course =).

libcxx/include/__format/format_arg.h
70

Microsoft STL pulled out monostate into a separate header to avoid including variant.

Mordante updated this revision to Diff 353419.Jun 21 2021, 10:47 AM

Tab -> space.

Mordante marked an inline comment as done.Jun 21 2021, 10:57 AM

LGTM but this will need to be reviewed by someone familiar with libc++ of course =).

Thanks for the review. Yeah the final approval will be done by someone more familiar with libc++.

libcxx/include/__format/format_arg.h
70

I plan to do the same. I added a todo at line 20.

Mordante updated this revision to Diff 353430.Jun 21 2021, 11:11 AM
Mordante marked an inline comment as done.

Forward declare visit_format_arg. Test whether this fixes the modular build.

Mordante updated this revision to Diff 353444.Jun 21 2021, 11:44 AM

Revert last change.
Included more headers and the test passes locally.

Mordante updated this revision to Diff 353669.Jun 22 2021, 9:11 AM

Adds some missing includes. Hopefully that fixes the GCC builds.

Mordante updated this revision to Diff 354005.Jun 23 2021, 9:54 AM

Disable the failing test on GCC.

Mordante updated this revision to Diff 354160.Jun 23 2021, 11:10 PM

Disable test gcc-10.
Attempt to fix the modular build.

Mordante updated this revision to Diff 354312.Jun 24 2021, 11:16 AM
Mordante added a subscriber: cjdb.

Fixes for the build. The test pass now locally both with a normal and a modular build.
Thanks to @ldionne and @cjdb for their input.

Mordante updated this revision to Diff 354432.Jun 24 2021, 11:02 PM

Rebase to trigger CI, the previous build failed due to a flacky test.
Use <__utility/forward.h> instead of <utility> in <__format/format_fwd.h>.

Mordante updated this revision to Diff 357273.Thu, Jul 8, 10:41 AM

Rebased.
Use new monostate header.

vitaut requested changes to this revision.Sun, Jul 11, 7:25 AM
vitaut added inline comments.
libcxx/include/__format/format_arg.h
233–235

An overload for [const] void* is missing.

239–240

In general basic_format_args are not comparable because they can refer to type-erased user-defined objects/formatters that can't be compared. If this is for testing I suggest moving to tests.

libcxx/include/__format/format_context.h
64

You could expose __uglified public "factory" functions for testing. MSVC is not a great example, they were doing a bunch of things wrong, I would reported this one if I had time.

libcxx/include/__format/format_fwd.h
2

What is the purpose of the format_fwd.h header?

libcxx/include/format
237

This could be a built-in array and the dependency on <array> removed. There is no advantage to using std::array here.

libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp
36–39

These checks seem overspecified especially considering that the representation will have to change to implement one of the TODOs. I'd recommend doing checks by converting __format_arg_store into basic_format_args and using the public API. Same elsewhere.

Another advantage of going through basic_format_args is that it will mimic the actual use.

libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp
88–109

All of this will have to change when we add char*_t support. I think we should static_assert in basic_format_context or elsewhere that CharT is char or wchar_t for now since other char type are not usable with <format> anyway and these conversions are morally wrong.

132

Create an alias for std::conditional_t<sizeof(long) == sizeof(int), int, long long> to avoid copy-paste?

198–217

ditto

281–291

Would be nice to add a test for basic_string_view with traits and basic_string with traits and an allocator.

libcxx/test/std/utilities/format/format.formatter/format.context/arg.pass.cpp
29 ↗(On Diff #357273)

nit: I'd call it output or result to distinguish from string above.

This revision now requires changes to proceed.Sun, Jul 11, 7:25 AM
Mordante updated this revision to Diff 358369.Tue, Jul 13, 11:29 AM
Mordante marked 9 inline comments as done.

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

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

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

libcxx/include/__format/format_arg.h
233–235

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.

239–240

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.

libcxx/include/__format/format_fwd.h
2

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

libcxx/include/format
237

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.

libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp
36–39

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.

libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp
88–109

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.

281–291

I really like this suggestion!

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

Now really rebase.

Mordante planned changes to this revision.Tue, Jul 13, 12:11 PM
Mordante added inline comments.Tue, Jul 13, 1:30 PM
libcxx/include/__format/format_context.h
64

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.Wed, Jul 14, 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.Wed, Jul 14, 10:05 AM
Mordante updated this revision to Diff 358682.Wed, Jul 14, 11:46 AM

Fixes localization build.
Rebased, hopefully fixes Apple builds.

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

Rebase to fix the GCC build.

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

LGTM

vitaut added inline comments.Sat, Jul 17, 6:28 AM
libcxx/include/__format/format_context.h
64

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.Sun, Jul 18, 11:07 AM

Guard unit tests for implementation details

Mordante added inline comments.Tue, Jul 20, 10:13 AM
libcxx/include/__format/format_arg.h
67

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

Quuxplusone added inline comments.
libcxx/include/__format/format_context.h
60

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 ( https://quuxplusone.github.io/blog/2020/04/18/default-function-arguments-are-the-devil ), 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.Fri, Jul 23, 12:28 PM

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

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

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

Mordante marked 3 inline comments as done.Sun, Jul 25, 7:10 AM
Mordante added inline comments.
libcxx/include/__format/format_context.h
60

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.