Skip to content

<format>: Make instantiation used by vformat_to lazier#3403

Merged
StephanTLavavej merged 4 commits intomicrosoft:mainfrom
frederick-vs-ja:lazy-vformat_to
Feb 14, 2023
Merged

<format>: Make instantiation used by vformat_to lazier#3403
StephanTLavavej merged 4 commits intomicrosoft:mainfrom
frederick-vs-ja:lazy-vformat_to

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Feb 11, 2023 •
edited
Loading

Fixes #1926.

The (current) cause of the issue is that currently _Parse_format_string<char, _Format_handler<char>> and its wchar_t variant are instantiated in vformat_to overloads (I'm not sure whether it's a bug of MSVC), and thus corresponding specializations of visit_format_arg are instantiated, so are _Fmt_write and eventually use_facet which touches locale::id.

I don't know how to write the test case for this...

It seems the MSVC STL implementation (or current standard specification?) is so type-erasing that we must pay the overhead for locale::id even when using locale-free format overloads.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner February 11, 2023 02:06
@cpplearner
Copy link
Contributor

cpplearner commented Feb 11, 2023 •
edited
Loading

The L specifier (which causes the argument to be formatted in the locale-specific form) can be used even when no locale is passed to std::format (in which case the global locale is used), and if L is specified, use_facet has to called to get e. g. the thousand separator, and use_facet needs the id to locate the facet. So I believe that format has to instantiate locale::id even if there's no type-erasing.

@StephanTLavavej StephanTLavavej added bug Something isn't working format C++20/23 format labels Feb 11, 2023
@jovibor
Copy link
Contributor

jovibor commented Feb 11, 2023

The L specifier (which causes the argument to be formatted in the locale-specific form) can be used even when no locale is passed to std::format (in which case the global locale is used)

Is this even legal?🤐
I mean, I always thought that the very purpose of the std::format(std::locale, overloads was exactly this, to save the user from all the locale's quirks...

What a brilliant design.
/offtopic

CaseyCarter
CaseyCarter reviewed Feb 12, 2023
View reviewed changes
@frederick-vs-ja
Merge from CaseyCarter/lazy-vformat_to
cd29d2a 
Casey: try to avoid messing with the interface of `vformat_to`
@frederick-vs-ja
Simplify _Dependent_char
c8ca089
@StephanTLavavej StephanTLavavej self-assigned this Feb 13, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Feb 13, 2023

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

@StephanTLavavej StephanTLavavej merged commit 5b29686 into microsoft:main Feb 14, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Feb 14, 2023

Thanks for helping the STL be a lazy kitty that flops on the floor and basks in sunbeams all day! 🐈 🌞 😻

@frederick-vs-ja frederick-vs-ja deleted the lazy-vformat_to branch February 14, 2023 01:11
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

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working format C++20/23 format

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<chrono>: Including header pulls in a dynamic initializer, incompatible with /kernel

5 participants

@frederick-vs-ja @cpplearner @jovibor @StephanTLavavej @CaseyCarter

Comments