Skip to content

<format>: Simplify type-erased argument storage#1907

Merged
StephanTLavavej merged 5 commits into
microsoft:mainfrom
CaseyCarter:format_arg_store
Jun 29, 2021
Merged

<format>: Simplify type-erased argument storage#1907
StephanTLavavej merged 5 commits into
microsoft:mainfrom
CaseyCarter:format_arg_store

Conversation

@CaseyCarter
Copy link
Copy Markdown
Contributor

@CaseyCarter CaseyCarter commented May 4, 2021 •
edited
Loading

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.

@CaseyCarter
Simplify type-erased format argument storage
dff667c 
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
@barcharcraz
Copy link
Copy Markdown
Contributor

barcharcraz 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

@CaseyCarter
Copy link
Copy Markdown
Contributor Author

CaseyCarter commented May 5, 2021 •
edited
Loading

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
barcharcraz
barcharcraz reviewed May 7, 2021
View reviewed changes
Comment thread stl/inc/format
barcharcraz
barcharcraz suggested changes May 11, 2021
View reviewed changes
Comment thread stl/inc/format Outdated
Comment thread stl/inc/format Outdated
Comment thread stl/inc/format Outdated
Comment thread stl/inc/format
Comment thread stl/inc/format
Comment thread stl/inc/format
@CaseyCarter CaseyCarter changed the title Simplify type-erased format argument storage <format>: Simplify type-erased argument storage May 25, 2021
@CaseyCarter CaseyCarter requested a review from barcharcraz June 8, 2021 21:39
barcharcraz
barcharcraz suggested changes Jun 12, 2021
View reviewed changes
Comment thread stl/inc/format Outdated
Comment thread stl/inc/format
StephanTLavavej
StephanTLavavej reviewed Jun 23, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread tests/std/tests/P0645R10_text_formatting_args/test.cpp Outdated
Comment thread stl/inc/format Outdated
@StephanTLavavej StephanTLavavej self-assigned this Jun 24, 2021
@StephanTLavavej StephanTLavavej merged commit 7ed04d9 into microsoft:main Jun 29, 2021
@StephanTLavavej
Copy link
Copy Markdown
Member

StephanTLavavej commented Jun 29, 2021

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

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@barcharcraz barcharcraz barcharcraz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

enhancement Something can be improved format C++20/23 format

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@CaseyCarter @barcharcraz @StephanTLavavej