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

Check make_format_args's preconditions #3080

Merged
merged 2 commits into from Oct 12, 2022

Conversation

cpplearner
Copy link
Contributor

[format.arg.store]

Preconditions: The type typename Context​::​template formatter_­type<Ti> meets the BasicFormatter requirements ([formatter.requirements]) for each Ti in Args.

Currently, this precondition isn't checked. If an user passes an unformattable type, the error message will likely start with "'std::_Format_arg_traits<_Context>::_Phony_basic_format_arg_constructor': none of the 5 overloads could convert all the argument types", which isn't quite helpful.

Before LWG-3701, It was impractical to check this precondition, because even std::format("Hello {}!\n", "world") was a precondition violation. This is no longer a concern after LWG-3701 lands.

The error message was copied from fmtlib (https://github.com/fmtlib/fmt/blob/1feb430faaac6bd8094e996861d6025f9903d34e/include/fmt/core.h#L1771-L1772).

@cpplearner cpplearner requested a review from a team as a code owner September 6, 2022 19:42
stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added enhancement Something can be improved format C++20/23 format labels Sep 7, 2022
@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews via automation Sep 7, 2022
@barcharcraz
Copy link
Member

I'm scared that this is going to blow up because we got the nuances of the type mapping machinery confused again, somehow. I'm worried about types where formatability depends on cvref qualifiers

@barcharcraz
Copy link
Member

I couldn't come up with any cases where this error does not agree with the later concept failure, and this actually is an error improvement.

@barcharcraz barcharcraz moved this from Initial Review to Final Review in Code Reviews Sep 22, 2022
@barcharcraz barcharcraz removed their assignment Sep 22, 2022
Co-authored-by: Sam Huang <samestimable2016@gmail.com>
@StephanTLavavej
Copy link
Member

I've added Standardese citations as recommended by @sam20908 (thanks!). Note that the same paragraph is cited for narrow and wide because of Effects Equivalent To wording.

✅ No modules impact.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 23, 2022
@StephanTLavavej StephanTLavavej self-assigned this Oct 11, 2022
@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 fe9f858 into microsoft:main Oct 12, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 12, 2022
@StephanTLavavej
Copy link
Member

Thanks for this diagnostic improvement! 🎉 💡 😸

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
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants