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>: Simplify type-erased argument storage #1907

Merged
merged 5 commits into from Jun 29, 2021

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented May 4, 2021

Rename the overload set that emulates the exposition-only constructors of basic_format_arg from _Get_format_arg_storage_type to _Phony_basic_format_arg_constructor for clarity. Encapsulate that overload set - and the traits _Storage_type and _Storage_size that observe it - in a new class template _Format_arg_traits. Remove the bogus monostate case and split the floating-point types out from the function template overload to more closely reflect the basic_format_arg constructors.

Shorten _Format_arg_store_packed_index to _Format_arg_index and make its converting constructor unconditionally noexcept.

In _Format_arg_store, store indices in an array using the untyped storage only for erased arguments. In _Store, reuse _Format_arg_traits::_Storage_type instead of trying to duplicate the overload resolution process.

Add test coverage for visit_format_arg(/**/, basic_format_arg</**/>()) to ensure that the monostate is properly visitable.

Rename the overload set that emulates the exposition-only constructors of `basic_format_arg` from `_Get_format_arg_storage_type` to `_Phony_basic_format_arg_constructor` for clarity. Encapsulate that overload set - and the traits `_Storage_type` and `_Storage_size` that observe it - in a new class template `_Format_arg_traits`. Remove the bogus `monostate` case and split the floating-point types out from the function template overload to more closely reflect the `basic_format_arg` constructors.

Shorten `_Format_arg_store_packed_index` to `_Format_arg_index` and make its converting constructor unconditionally `noexcept`.

In `_Format_arg_store`, store indices in an array using the untyped storage only for erased arguments. In `_Store`, reuse `_Format_arg_traits::_Storage_type` instead of trying to duplicate the overload resolution process.
@CaseyCarter CaseyCarter added the enhancement Something can be improved label May 4, 2021
@CaseyCarter CaseyCarter requested a review from a team as a code owner May 4, 2021 15:31
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation May 4, 2021
@barcharcraz
Copy link
Member

I thought you could store monostates and observe them through visit_format_arg? If that's no longer the case then we should also move monostate back to

@CaseyCarter
Copy link
Member Author

CaseyCarter commented May 5, 2021

I thought you could store monostates and observe them through visit_format_arg? If that's no longer the case then we should also move monostate back to

You can visit_format_arg(my_visitor, basic_format_arg<some_context>()), which notably doesn't involve basic_format_args or _Format_arg_store. Users create a _Format_arg_store by passing arguments to make_format_args; there's no argument they can pass that results in storing a monostate. (Note that monostate itself is not a valid argument type for make_format_args because there's no enabled formatter specialization for monostate.)

EDIT: I note that we are missing test coverage of visit_format_arg(my_visitor, basic_format_arg<some_context>()), which makes me nervous - I'll add some.

@StephanTLavavej StephanTLavavej added the format C++20/23 format label May 7, 2021
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Show resolved Hide resolved
stl/inc/format Show resolved Hide resolved
stl/inc/format Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress May 11, 2021
@CaseyCarter CaseyCarter changed the title Simplify type-erased format argument storage <format>: Simplify type-erased argument storage May 25, 2021
@CaseyCarter CaseyCarter moved this from Work In Progress to Initial Review in Code Reviews Jun 8, 2021
stl/inc/format Outdated Show resolved Hide resolved
stl/inc/format Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Jun 12, 2021
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in Code Reviews Jun 16, 2021
@barcharcraz barcharcraz added this to @barcharcraz in Maintainer Priorities Jun 16, 2021
@CaseyCarter CaseyCarter moved this from Initial Review to Final Review in Code Reviews Jun 22, 2021
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Looks good - I will validate and push changes for ultra-trivial nitpicks.

tests/std/tests/P0645R10_text_formatting_args/test.cpp Outdated Show resolved Hide resolved
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Jun 23, 2021
@StephanTLavavej StephanTLavavej self-assigned this Jun 24, 2021
@StephanTLavavej StephanTLavavej merged commit 7ed04d9 into microsoft:main Jun 29, 2021
Code Reviews automation moved this from Ready To Merge to Done Jun 29, 2021
@StephanTLavavej
Copy link
Member

Thanks for simplifying <format> - a net deletion of 56 lines of product code! 😻 ✏️

@CaseyCarter CaseyCarter deleted the format_arg_store branch June 29, 2021 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved format C++20/23 format
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants