This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Use forwarding references.
ClosedPublic

Authored by Mordante on Jun 11 2022, 8:07 AM.

Details

Reviewers
ldionne
vitaut
Group Reviewers
Restricted Project
Commits
rG606e280811f2: [libc++][format] Use forwarding references.
Summary

This implements a not accepted LWG issue. Not doing so would require
integral types to use the handle class instead of being directly stored
in the basic_format_arg.

The previous code used std::forward in places where it wasn't required
by the Standard. These are now removed.

Implements:

  • P2418R2 Add support for std::generator-like types to std::format
  • LWG 3631 basic_format_arg(T&&) should use remove_cvref_t<T> throughout

Diff Detail

Event Timeline

Mordante created this revision.Jun 11 2022, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2022, 8:07 AM
Mordante updated this revision to Diff 436158.Jun 11 2022, 12:43 PM

Add a separate test for move only types.

Mordante updated this revision to Diff 436197.Jun 12 2022, 2:21 AM
Mordante edited the summary of this revision. (Show Details)

Fixes CI and minor cleanups.

Mordante published this revision for review.Jun 12 2022, 3:44 AM
Mordante added reviewers: ldionne, vitaut.
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2022, 3:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jun 13 2022, 8:12 AM

LGTM with a question.

libcxx/include/__format/format_arg.h
261

IIUC this means that we can't construct a handle from a temporary anymore, however I don't think I saw any tests that needed to change in reaction to that -- is that expected?

libcxx/test/std/utilities/format/format.arguments/format.arg.store/make_format_args.pass.cpp
27–29

You could also use LIBCPP_STATIC_ASSERT if you prefer.

This revision is now accepted and ready to land.Jun 13 2022, 8:12 AM
Mordante marked 2 inline comments as done.Jun 14 2022, 10:51 AM
Mordante added inline comments.
libcxx/include/__format/format_arg.h
261

We can still construct a handle using a temporary, there are test for that. However the temporary is "stored" in __basic_format_arg_value<_Context>::__handle and not in this object.

libcxx/test/std/utilities/format/format.arguments/format.arg.store/make_format_args.pass.cpp
27–29

Good one! I forgot about that macro.

Mordante updated this revision to Diff 436854.Jun 14 2022, 10:51 AM
Mordante marked 2 inline comments as done.

Rebased and addresses review comments.

vitaut added inline comments.Jun 25 2022, 7:06 AM
libcxx/include/__format/format_arg.h
213–216

Why pass __handle by rvalue or call forward here? Note that handle is just a pair of pointers so it probably makes more sense to pass it by value as before.

libcxx/test/std/utilities/format/format.functions/P2418.pass.cpp
32

format should be const.

Mordante marked 2 inline comments as done.Jun 25 2022, 10:50 AM

Thanks for the review!

libcxx/include/__format/format_arg.h
213–216

There's something odd, I can remove the references. But when I remove the std::forward it breaks. I will investigate this at a later time.

libcxx/test/std/utilities/format/format.functions/P2418.pass.cpp
32

Good catch! Unfortunately std::formatter<int, CharT>::format isn't const qualified yet. That's WIP and will be fixed soon. Instead I'll add a TODO.

Mordante updated this revision to Diff 440000.Jun 25 2022, 11:11 AM
Mordante marked 2 inline comments as done.

Rebased
Addresses review comments
Updates the release notes to the latest style

This revision was automatically updated to reflect the committed changes.

This PR didn't update the format status page. Is the warning from C++20 status still current?

P0645: The paper is implemented but still marked as an incomplete feature (the feature-test macro is not set and the libary is only available when built with LIBCXX_ENABLE_INCOMPLETE_FEATURES). Not yet implemented LWG-issues will cause API and ABI breakage.

I would have though that the chrono-integration is orthogonal to the ABI issues.

This PR didn't update the format status page. Is the warning from C++20 status still current?

P0645: The paper is implemented but still marked as an incomplete feature (the feature-test macro is not set and the libary is only available when built with LIBCXX_ENABLE_INCOMPLETE_FEATURES). Not yet implemented LWG-issues will cause API and ABI breakage.

I would have though that the chrono-integration is orthogonal to the ABI issues.

I'm quite sure I indeed can do chono without an ABI break. But https://wg21.link/p2572r0 will be ABI breaking for at least char. If I really wanted, I probably can work around it. But since the paper hasn't been seen by LEWG I don't know how it will be received there. I also intend to look at some other improvements, this will be a lot easier when we're not ABI frozen.

However when we have -fexperimental-library I can set the feature-test macro to the value for P2418. That's already on my todo list. Having it in experimental will make it a lot easier to use format, so I'm quite happy about that.