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

Implement LWG-3631 basic_format_arg(T&&) should use remove_cvref_t<T> throughout #3745

Merged
merged 3 commits into from
Jun 15, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Jun 3, 2023

Also make formerly exposition only constructors private. As a result, several class templates are forward declared.

Add test coverage for construction of handle and inaccessibility of exposition only constructor(s). Note that construction of handle from a const rvalue is still possible since a const T rvalue can be bound to const T&.

Per @cpplearner's discovery on Discord, I decided to ensure that TQ (_Qual_for_handle<_Ty>_Tq in this PR) never drops const-qualifier from T (_Ty).

Fixes #3460.

Also make formerly exposition only constructors private.

Add test coverage for construction of `handle` and inaccessibility of exposition only constructor(s).
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner June 3, 2023 17:26
Copy link
Member

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

This changes behavior pretty radically in the case of stuff that has conversions to one of the types supported "natively" by basic_format_arg, I actually like the change but just want to note that potentially breaking change. It's probably good though, since now such types can get formatted by their own formatter.

Requesting only the change to inline _Qual_for_handle

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

@barcharcraz barcharcraz left a comment

Choose a reason for hiding this comment

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

This changes behavior pretty radically in the case of stuff that has conversions to one of the types supported "natively" by basic_format_arg, I actually like the change but just want to note that potentially breaking change. It's probably good though, since now such types can get formatted by their own formatter.

Requesting only the change to inline _Qual_for_handle

Inlining `_Qual_for_handle`.
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Jun 7, 2023
@StephanTLavavej StephanTLavavej added the format C++20/23 format label Jun 7, 2023
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks! FYI @barcharcraz I pushed trivial changes after you approved.

@StephanTLavavej StephanTLavavej removed their assignment Jun 10, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jun 14, 2023
@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 f68e64c into microsoft:main Jun 15, 2023
@StephanTLavavej
Copy link
Member

Thanks for improving the usability of <format>! 😻 📝 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format C++20/23 format LWG Library Working Group issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LWG-3631 basic_format_arg(T&&) should use remove_cvref_t<T> throughout
3 participants