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

Support char8_t in C++17 and C++14 modes #2748

Merged
merged 15 commits into from
Jun 12, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented May 29, 2022

Fixes #2106

@fsb4000 fsb4000 marked this pull request as ready for review May 29, 2022 11:20
@fsb4000 fsb4000 requested a review from a team as a code owner May 29, 2022 11:20
@frederick-vs-ja
Copy link
Contributor

Is there any reason to accept (or regularize) the "C++17 with char8_t" dialect, but not "C++14 with char8_t"?

@fsb4000
Copy link
Contributor Author

fsb4000 commented May 29, 2022

Probably because <filesystem> and <string_view> are C++17.
But it seems u8string works in C++14

@fsb4000 fsb4000 changed the title Add test coverage /std:c++17 /Zc:char8_t Support char8_t in C++17 and C++14 modes May 29, 2022
@fsb4000
Copy link
Contributor Author

fsb4000 commented May 29, 2022

Maybe I should create char8_t_matrix.lst and char8_t_17_matrix.lst instead of adding the configuration to usual matrixes, shouldn't I?

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label May 29, 2022
@StephanTLavavej
Copy link
Member

Yeah, changes to the usual matrices are extremely expensive because the cost of each configuration is multiplied by the large number of tests. Also, when they're changed, typically the native matrix and others need to be changed.

Since relatively little code is sensitive to this setting, I like your idea of creating a new matrix (or matrices) for a new test (or a small number of new tests). A single matrix with both C++14 and C++17 modes, with a test that has C++17-only components guarded by _HAS_CXX17, might be reasonable.

@fsb4000 fsb4000 marked this pull request as draft May 30, 2022 06:32
@frederick-vs-ja
Copy link
Contributor

IMO we should also consider deprecation of u8path:

  • if there's no char8_t, we should not deprecate it,
  • otherwise, we should determine whether it is deprecated in C++17 with char8_t mode.

@fsb4000 fsb4000 marked this pull request as ready for review June 2, 2022 19:48
@StephanTLavavej
Copy link
Member

Thanks, this looks great! I verified that all std tests mentioning char8_t, u8string, u8string_view, __cpp_char8_t, and __cpp_lib_char8_t have been updated to use the new matrices, except for tests that are inherently C++20/23. I pushed a comment change to yvals_core.h to reflect the feature-test macros, and updated the feature-test macro test's configurations to exercise the new codepath.

stl/inc/yvals_core.h Show resolved Hide resolved
tests/std/tests/char8_t_17_matrix.lst Outdated Show resolved Hide resolved
tests/std/tests/char8_t_matrix.lst Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter self-assigned this Jun 8, 2022
Co-authored-by: Casey Carter <[email protected]>
@CaseyCarter CaseyCarter removed their assignment Jun 9, 2022
@StephanTLavavej StephanTLavavej self-assigned this Jun 10, 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 37b5120 into microsoft:main Jun 12, 2022
@StephanTLavavej
Copy link
Member

Thanks for making this Future Technology available to C++14/17 users! 🚀 🛸 😸

@fsb4000 fsb4000 deleted the fix2106 branch June 12, 2022 09:38
fsb4000 added a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
Co-authored-by: Stephan T. Lavavej <[email protected]>
Co-authored-by: Casey Carter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support char8_t in C++17 mode
4 participants