This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Add __format_arg_store.
AbandonedPublic

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.

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
227

Typo: implemented.

229

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
227

Thanks will fix.

229

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.Feb 1 2021, 11:33 AM
Mordante updated this revision to Diff 320543.Feb 1 2021, 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.Feb 1 2021, 11:42 AM

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

Mordante updated this revision to Diff 320839.Feb 2 2021, 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.Feb 2 2021, 10:26 AM

More changes will be needed.

Mordante updated this revision to Diff 321456.Feb 4 2021, 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.Feb 4 2021, 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
223

@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.Feb 6 2021, 4:14 AM

Rebased to trigger CI.

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

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

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

Need to solve the the CI merge issue first.

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

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.Feb 10 2021, 9:10 AM
Mordante added inline comments.
libcxx/include/format
223

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.Feb 11 2021, 9:10 AM
Mordante marked an inline comment as done.

Rebased to trigger CI.

Mordante updated this revision to Diff 323538.Feb 13 2021, 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.Feb 13 2021, 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.Feb 13 2021, 4:44 AM

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

Mordante updated this revision to Diff 323552.Feb 13 2021, 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.Feb 20 2021, 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.Feb 21 2021, 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
415–425

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 )

510–513

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

cjdb added inline comments.Feb 21 2021, 12:14 AM
libcxx/include/format
415–425

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

tschuett added a subscriber: tschuett.EditedFeb 21 2021, 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.Feb 21 2021, 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
415–425

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

510–513

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.Feb 21 2021, 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.Feb 21 2021, 6:18 AM
Mordante updated this revision to Diff 339741.Apr 22 2021, 11:38 AM

Rebased and adjusted due to D100736 [libc++] s/_LIBCPP_NO_HAS_CHAR8_T/_LIBCPP_HAS_NO_CHAR8_T/g

Mordante planned changes to this revision.Apr 22 2021, 11:46 AM

Due to https://wg21.link/P2216 some functions need to be disabled on Apple.

Very small comments, leading presumably to a lot of changes in the next revision. I didn't review closely or anything.

libcxx/include/format
362–367

Nit throughout: _VSTD::forward (and all other functions of >=1 argument) because ADL.
But you don't need (and therefore shouldn't put) _VSTD:: on concepts, classes, or variables; i.e. you should write monostate not _VSTD::monostate, and same_as not _VSTD::same_as, and so on.
Essentially, just as you should think of explicit as meaning "Here comes a constructor!", you should think of _VSTD:: in the libc++ codebase as meaning "Here comes a function call!"

Hopefully this fixes line 442's )&&sizeof(_Tp) misformatting, as well.

482–483

This being a "Precondition", I think it'd be better to use _LIBCPP_ASSERT here. Users of libc++ shouldn't be taught to rely on this exception happening, because it'll probably stop happening as soon (heh) as Contracts lands.

Mordante updated this revision to Diff 340256.Apr 24 2021, 2:40 AM

Adresses review comments:

  • Improves usage of _VSTD
  • Use a _LIBCPP_ASSERT to validate a precondition instead of an exception
Mordante updated this revision to Diff 340262.Apr 24 2021, 4:11 AM

Rebase to trigger CI. Just to make sure the build error isn't related to other recent build errors.

Mordante updated this revision to Diff 340265.Apr 24 2021, 4:41 AM

Use _VSTD::locale to fix GCC errors.

Mordante updated this revision to Diff 340274.Apr 24 2021, 6:55 AM

Fixes an ambiguity in the template overload resolution when char is unsigned.
This probably fixes the AArch64 and Arm build failures.

Funny enough there is [[deprecated("message")]], but there is no [[unstable("message")]].

Mordante planned changes to this revision.Apr 24 2021, 11:08 AM
Mordante marked 2 inline comments as done.

Funny enough there is [[deprecated("message")]], but there is no [[unstable("message")]].

:-)

For the CI passes. Still need to look at the attributes. I'll also look at polishing the code a bit further.

libcxx/include/format
362–367

Good point. I addressed these issues.
The formatting is still not great. Guess we need to wait for clang-format to properly implement concepts formatting.

Mordante updated this revision to Diff 347200.May 22 2021, 5:56 AM
Mordante marked an inline comment as done.

Major patch rework

  • Separated in multiple headers
  • Removed the variant
  • Several minor improvements

Note the patch isn't ready for review yet, but getting close.

Mordante planned changes to this revision.May 22 2021, 5:57 AM
Mordante edited the summary of this revision. (Show Details)May 22 2021, 6:15 AM
Mordante updated this revision to Diff 347203.May 22 2021, 6:38 AM

Rebase to trigger CI.

Mordante planned changes to this revision.May 22 2021, 6:39 AM
Mordante abandoned this revision.Jun 5 2021, 4:01 AM

This patch has been abandoned in favour of D103357. Some of the prerequisites have landed which causes the CI to have merge issues.