Implement P2508R1 basic_format_string, format_string, wformat_string #3074
Conversation
|
Ah... I've started working on this in an alternative way which derives |
|
Or alternatively, should we implement |
|
I will implement the alternative approaches if I have to, but I really hope that we don't need them. |
StephanTLavavej
added
the
decision needed
|
Maintainer My initial reaction is that this should be guarded for C++23, as we generally consider the introduction of new identifiers to be a bright line. However, perhaps the signature change is sufficiently invasive, and the risk of conflict sufficiently low, that implementing this "downlevel" would be reasonable. Certainly, something that affects only C++20 is much less risky than something that goes back to C++14/17. One more note: we won't be backporting further changes to VS 2019 16.11.x except in highly critical cases; I believe that this is an additional reason to avoid changing C++20 behavior. |
|
Note that P2419R2 "Clarify handling of encodings in localized formatting of chrono types" was implemented for C++20. If P2508R1 is guarded for C++23, then it needs to be decided whether |
|
It's a bit strange that while P2419R2 looks like a DR (resolving LWG-3565) but P2508R1 doesn't, both papers bump the same feature-test macro. I plan to contact LWG for clarification. |
StephanTLavavej
removed
the
decision needed
|
Charlie and I talked about this at the weekly maintainer meeting and we're ok with the current approach of implementing this in C++20 mode - thanks! |
There is currently a discussion on the SG-10 mailing list about this and there is an LWG issue FYI libc++ has also implemented this paper in C++20 mode. This is shipped in LLVM 15. |
Yeah, if libc++ already did it in C++20 mode it seems like we should too. Gating it behind C++23 is likely to cause much more pain for people using multiple compilers than the new identifier. |
|
In libc++ we never shipped the exposition only version. So it was easy to decide to do it this way. |
|
Looks good - I see nothing missing here, and the risk of linker errors when mixing old and new code is low. (Previously, the type couldn't be directly named, so it appeared as a function parameter and would be implicitly constructed. The risk would be higher if functions could have easily returned the type.) The only thing I considered commenting on was that the |
There was a problem hiding this comment.
This LGTM, but I'm leaving in Final Review so @barcharcraz can take a look.
| template <class charT> | ||
| constexpr void test_basic_format_string() { | ||
| { | ||
| basic_format_string<charT> fmt_str = basic_string_view{STR("meow")}; | ||
| assert(fmt_str.get() == STR("meow")); | ||
| } | ||
| { | ||
| basic_format_string<charT, double, int> fmt_str = STR("{:a} {:b}"); | ||
| assert(fmt_str.get() == STR("{:a} {:b}")); | ||
| } | ||
| } | ||
|
|
||
| constexpr bool test_format_string() { | ||
| test_basic_format_string<char>(); | ||
| test_basic_format_string<wchar_t>(); | ||
|
|
||
| static_assert(is_same_v<format_string<int*>, basic_format_string<char, int*>>); | ||
| static_assert(is_same_v<wformat_string<int*>, basic_format_string<wchar_t, int*>>); | ||
|
|
||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
These tests are pretty low value IMO, but since you already wrote them I'm fine with using them.
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
Thanks for implementing this C++23 feature and increasing |
This implements P2508R1.
Fixes #2937.
The paper mentions that
But I strongly hope that this can be applied to C++20, so the signature of
std::formatdoes not change between C++20 and C++23.