Page MenuHomePhabricator

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

Authored by Mordante on Dec 20 2020, 4:36 AM.

Details

Reviewers
EricWF
ldionne
mclow.lists
miscco
curdeius
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 make_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 when there's libc++ support for 128 bit integrals.

Implements parts of:

  • P0645 Text Formatting

Completes:

  • LWG3371 visit_format_arg and make_format_args are not hidden friends

Note it's possible to make the code more efficient, but for now the focus
is to get a Standard conforming implementation. Improving the QoI will be
done later.

Note the formatting is done with clang-format default settings. Should we
improve these settings or do we prefer to manually adjust the code and
protect it with clang-format: off comments?

Depends on D93166

Diff Detail

Event Timeline

Mordante requested review of this revision.Dec 20 2020, 4:36 AM
Mordante created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2020, 4:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

It seems the CI build fails since it can't process the dependencies properly. I'll rebase and trigger an CI build once the dependencies have landed.

Mordante planned changes to this revision.Jan 22 2021, 8:35 AM
curdeius added inline comments.Jan 26 2021, 7:32 AM
libcxx/include/format
237

Typo: implemented.

239

We should make sure not to define __cpp_lib_concepts before having these concepts implemented then (or better, have all concepts implemented).
Personally, I'd rather just use below defined concepts and get rid of this ifdef.

Thanks for the review.

libcxx/include/format
237

Thanks will fix.

239

We should make sure not to define __cpp_lib_concepts before having these concepts implemented then (or better, have all concepts implemented).

Agreed.

Personally, I'd rather just use below defined concepts and get rid of this ifdef.

I've no objection. That also makes it possible to switch to specific concepts once they've landed. The (un)signed_integral is already in review.

Mordante marked 2 inline comments as done.Mon, Feb 1, 11:33 AM
Mordante updated this revision to Diff 320543.Mon, Feb 1, 11:37 AM
Mordante edited the summary of this revision. (Show Details)
  • Fixed spelling errors
  • Only depend on our own concepts and no longer use #ifdef for not yet implement ed concepts.
  • Use MKSTR instead of make_string
  • Added synopsis
  • Targeted release paper for libc++ 13 instead of 12.
  • Ran clang-format (fixes formatting of if constexpr)
  • Rebased and use __throw_format_error instead of __format::__throw_error
Mordante planned changes to this revision.Mon, Feb 1, 11:42 AM

There seem to be build issues, so will investigate them.

Mordante updated this revision to Diff 320839.Tue, Feb 2, 10:25 AM
  • Fixes documentation formatting error.
  • Use a layer of indirection to avoid inconsistent default arguments, probably fixes GCC's build failure.
  • Don't use temporary strings in std::make_format_args, probably fixes the sanitizer build issues.
  • The filesystem issue will be solved in a different way.
Mordante planned changes to this revision.Tue, Feb 2, 10:26 AM

More changes will be needed.

Mordante updated this revision to Diff 321456.Thu, Feb 4, 8:41 AM
  • Fixes the sanitizer issues, this probably fixes the GCC issue.
  • It seems AppleClang 12 doesn't support concepts, first test whether disabling them works.
Mordante planned changes to this revision.Thu, Feb 4, 8:45 AM
Mordante added a subscriber: mehdi_amini.

Wanted to have a CI run to test some fixes, more work still needs to be done.

libcxx/include/format
233

@ldionne If you agree with this approach I'll move it to D93166, which then should fix the issues @mehdi_amini has using clang 8.

libcxx/test/std/utilities/format/format.arguments/format.arg/visit_format_arg.pass.cpp
52

MKSTR -> MAKE_STRING and all other occurrences.

Mordante updated this revision to Diff 321925.Sat, Feb 6, 4:14 AM

Rebased to trigger CI.

Mordante updated this revision to Diff 321927.Sat, Feb 6, 4:35 AM

Switch to MAKE_STRING instead of MKSTR, this should fix the filesystem build.

Mordante planned changes to this revision.Sat, Feb 6, 4:40 AM

Need to solve the the CI merge issue first.

ldionne added inline comments.Tue, Feb 9, 3:50 PM
libcxx/include/format
233

Ok, but please add a fat TODO here.

It's now my #1 priority to bump the compiler requirements for building libc++. It just doesn't make sense to try to build a C++20 library with a compiler that doesn't support C++20. The resulting library will be broken. Plus we've been accumulating serious debt lately due to this outdated policy.

For now make sure it works with Clang 8, but I'll kick this off on llvm-dev and the other lists.

@mehdi_amini You should move to building libc++ with the runtimes build (i.e. building libc++ with the Clang you just bootstrapped) to avoid issues like this in the future. Building libc++ with Clangs that old is only supported on paper, but I would never do that. I'll work on making the policy meet reality.

Mordante marked an inline comment as done.Wed, Feb 10, 9:10 AM
Mordante added inline comments.
libcxx/include/format
233

Ok, but please add a fat TODO here.

Will do

It's now my #1 priority to bump the compiler requirements for building libc++. It just doesn't make sense to try to build a C++20 library with a compiler that doesn't support C++20. The resulting library will be broken. Plus we've been accumulating serious debt lately due to this outdated policy.

It would indeed be great if we can change this policy. Thanks for working on it.

Mordante updated this revision to Diff 323044.Thu, Feb 11, 9:10 AM
Mordante marked an inline comment as done.

Rebased to trigger CI.

Mordante updated this revision to Diff 323538.Sat, Feb 13, 2:30 AM

Removed so obsolete comment.
Use the arithmetic concepts directly since they've landed in main.

Mordante updated this revision to Diff 323541.Sat, Feb 13, 3:07 AM

Make _LIBCPP_HAS_NO_CONCEPTS not GCC specific. (The lack of indention made it hard to detect it was a GCC specific section.)

Mordante updated this revision to Diff 323548.Sat, Feb 13, 4:44 AM

Let all unit test require concepts. Test whether that fixes the Apple build issues.

Mordante updated this revision to Diff 323552.Sat, Feb 13, 5:56 AM

The concept fix for Apple seems to be working, but needs to be enabled for some existing tests.

Mordante updated this revision to Diff 325188.Sat, Feb 20, 4:12 AM

Rebased on main to merge the already committed part.
@ldionne can you have a look at this patch it should be ready.

miscco added a subscriber: cjdb.Sun, Feb 21, 12:07 AM

Thanks for working on this. It is a considerable library

I am really concerned about doing a "quick" implementation and then being locked in due to ABI issues.

I believe we should spend the time up front to get a clean efficient implementation

libcxx/include/format
418–428

I think we whould really constraint this on template<integral _Tp> and handle all integral types here. Having them as multiple separat functions has significant maintainance overhead. Same for <template<floating_point _Tp>

I think they are in flight by @cjdb so maybe wait a bit to get this in.

(FYI this i how it I dit it for MSVC STL https://github.com/microsoft/STL/blob/99e8a2d7f2a083a6f768cff35a5e1792feb625a6/stl/inc/format#L933-L962 )

513–516

I *think* we should be able to default this as it is C++20

cjdb added inline comments.Sun, Feb 21, 12:14 AM
libcxx/include/format
418–428

same_as, integral, and floating_point are all merged :-)

tschuett added a subscriber: tschuett.EditedSun, Feb 21, 12:16 AM

You could put a comment at the top of the format header:
This library is still in development and there are no stability guarantees.

Or if you dare:

#warning the format library is still in development
Mordante marked an inline comment as done.Sun, Feb 21, 5:58 AM

Thanks for working on this. It is a considerable library

I am really concerned about doing a "quick" implementation and then being locked in due to ABI issues.

I believe we should spend the time up front to get a clean efficient implementation

In general I agree. However the ABI will most likely be broken. Victor contacted me and http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2216r2.html will most likely be accepted for C++23 and retroactively be part of C++20. I'll will discuss this on Slack in the near future.

I also like to have some of the basics in so I can add benchmarks to see whether optimizations have a positive effect.

I already added a warning in the release document that the ABI isn't stable yet. Until it is I don't want to set __cpp_lib_format.

What do you think @ldionne?

You could put a comment at the top of the format header:
This library is still in development and there are no stability guarantees.

That sounds like a good idea.

Or if you dare:

#warning the format library is still in development

I'm quite sure that will fail in our lit tests ;-)

libcxx/include/format
418–428

I think we whould really constraint this on template<integral _Tp> and handle all integral types here. Having them as multiple separat functions has significant maintainance overhead. Same for <template<floating_point _Tp>

I'll have a look at we can combine. But I think here we need some separate functions.

I think they are in flight by @cjdb so maybe wait a bit to get this in.

Yes I've already used some of the newer concepts recently added. But it seems I've missed same_as.

(FYI this i how it I dit it for MSVC STL https://github.com/microsoft/STL/blob/99e8a2d7f2a083a6f768cff35a5e1792feb625a6/stl/inc/format#L933-L962 )

Thanks for the link. It seems MSVC has more overloads for this part https://github.com/microsoft/STL/blob/99e8a2d7f2a083a6f768cff35a5e1792feb625a6/stl/inc/format#L195
(Obviously it's required there since Microsoft doesn't use a variant.)

513–516

I think that should be possible in theory. However I'm not sure how well compiler support for this feature is. Clang hasn't implemented all related papers yet. Since it's trivial to write the function manually I've done so. I'm not sure whether the performance of the variant is good enough. If not I need to change it to something else.

Mordante updated this revision to Diff 325288.Sun, Feb 21, 6:17 AM
Mordante marked an inline comment as done.
  • Added a warning as proposed by @tschuett
  • Use more concepts as suggested by @miscco
  • Rebased on main
Mordante marked 2 inline comments as done.Sun, Feb 21, 6:18 AM