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-3720 Restrict the valid types of arg-id for width and precision in std-format-spec #3511

Merged
merged 5 commits into from Mar 3, 2023

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Feb 28, 2023

Fixes #3425

@fsb4000 fsb4000 requested a review from a team as a code owner February 28, 2023 06:39
@github-actions github-actions bot added this to Initial Review in Code Reviews Feb 28, 2023
@fsb4000
Copy link
Contributor Author

fsb4000 commented Feb 28, 2023

std::format(L"{:*^{}}", 'a', '0');

'0' is type erasured as wchar_t, (_Arg == Char state)
and later it passes the condition _Standard_signed_or_unsigned_integer<_Ty> because wchar_t == unsigned short

I'm not sure what we should do with /Zc:wchar_t-

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Feb 28, 2023

I think we can still tell char_t from standard signed/unsigned integer types on /Zc:wchar_t-, because signed/unsigned integer types are never type-erased as unsigned short (only int, unsigned int, long long, and unsigned long long would be the erased type).

The real problem, IMO, is that it's impossible to conform to both [format.arg]/6.1 and [format.arg]/6.4 when wchar_t is same as unsigned short.

@fsb4000
Copy link
Contributor Author

fsb4000 commented Feb 28, 2023

Thank @frederick-vs-ja !

Ok, I got you, so this code is accepted with /Zc:wchar_t- (and it throws without /Zc:wchar_t-)

std::wstring s = std::format(L"{:*^{}}\n", 'a', L'0');

and the lwg issue has the sentence:

The following integral types are rejected by both libc++ and MSVC STL:

std::cout << std::format("{:*^{}}\n", 'a', L'0');

@CaseyCarter CaseyCarter added the LWG Library Working Group issue label Feb 28, 2023
stl/inc/format Outdated Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Feb 28, 2023
@CaseyCarter CaseyCarter moved this from Work In Progress to Final Review in Code Reviews Feb 28, 2023
@strega-nil-ms strega-nil-ms moved this from Final Review to Ready To Merge in Code Reviews Feb 28, 2023
@StephanTLavavej
Copy link
Member

This works because we have machinery that promotes tiny types before they're seen here. The test coverage for signed char is sufficient.

@StephanTLavavej StephanTLavavej self-assigned this Mar 2, 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 efd50e8 into microsoft:main Mar 3, 2023
Code Reviews automation moved this from Ready To Merge to Done Mar 3, 2023
@StephanTLavavej
Copy link
Member

Thanks for the precise PR! 🎯 🎉 😸

@fsb4000 fsb4000 deleted the fix3425 branch March 4, 2023 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LWG Library Working Group issue
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

LWG-3720 Restrict the valid types of arg-id for width and precision in std-format-spec
5 participants