Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<format>: Implement P2418R2 support for std::generator-like types #2323

Merged
merged 14 commits into from
Jan 26, 2022

Conversation

barcharcraz
Copy link
Member

@barcharcraz barcharcraz commented Nov 4, 2021

Implements P2418R2 which prepares format to be able to format std::generators, by allowing "format" functions that take their parameters by non-const reference. This DR is somewhat high risk because it modifies the argument type mapping machinery.

  • make(w)_format_args was modified to forward its arguments, I think this is what the standard says to do but I'm not 100% sure since the standard has make_format_args directly initialize each basic_format_arg while we need to pass the arguments to all together to _Format_arg_store.
  • I have modified usages of _Storage_type to use remove_cvref_t<T> instead of T because that's what the "other end" (the un-erase from handle) uses now. I don't think this has actual effect on the mapping, but could change the selected formatter specialization for custom types. Tim Song indicated on the reflectors that the lack of remove_cvref_t<T> in [format.arg]/5 is probably a defect.
  • The _Has_formatter and the new _Has_const_formatter concepts have been moved up since they are now needed in the constructor for basic_format_arg::handle

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2418r2.html

Fixes #2239.

@barcharcraz barcharcraz added the defect report Applied retroactively label Nov 4, 2021
@barcharcraz barcharcraz requested a review from a team as a code owner November 4, 2021 01:53
@StephanTLavavej StephanTLavavej added cxx20 C++20 feature format C++20/23 format labels Nov 4, 2021
@StephanTLavavej StephanTLavavej changed the title <format>: Implement P2418R2 <format>: Implement P2418R2 support for std::generator-like types Nov 4, 2021
stl/inc/format Show resolved Hide resolved
stl/inc/format Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej

This comment has been minimized.

@barcharcraz

This comment has been minimized.

@StephanTLavavej StephanTLavavej self-assigned this Jan 12, 2022
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated
_NODISCARD auto make_format_args(const _Args&... _Vals) {
return _Format_arg_store<_Context, _Args...>{_Vals...};
_NODISCARD auto make_format_args(_Args&&... _Vals) {
return _Format_arg_store<_Context, _Args...>{_STD forward<_Args>(_Vals)...};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we forwarding when the Working Draft doesn't show forwarding? (Also on 2966.) If we don't forward here, the _Format_arg_store constructor can/should be changed to take only lvalues.

Have you submitted an LWG issue to fix the preconditions on these functions to strip cvref-quals from the Ti?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm forwarding because libfmt forwards here, I think you're right though and they forward everywhere but then specifically undo what the forwarding would have done when make_arg binds them all to the fmt equivalent to const _Storage_type<remove_cvref_t<T>>&

Have you submitted an LWG issue to fix the preconditions on these functions to strip cvref-quals from the Ti?

no, but Tim Song did.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait is this a separate issue from LWG-3631?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is - [format.arg.store]/2. We can do this as a followup - it doesn't need to block merging.

stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I pushed a conflict-free merge with main, in addition to a comment change, because there was the major toolset update containing Clang 13. I verified that no additional reformatting was necessary.

@StephanTLavavej StephanTLavavej removed their assignment Jan 22, 2022
@CaseyCarter CaseyCarter self-requested a review January 24, 2022 05:54
@CaseyCarter CaseyCarter removed their assignment Jan 24, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jan 26, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit f0c0eaa into microsoft:main Jan 26, 2022
@StephanTLavavej
Copy link
Member

Thanks for implementing the last C++20 DR! 🎉 😻 ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature defect report Applied retroactively format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2418R2 Add Support For std::generator-like Types To std::format
4 participants