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

__cpp_char8_t does not cover std::u8string implementation #4182

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

venik
Copy link
Contributor

@venik venik commented Mar 7, 2023

In MSVS std::u8string is enabled only when __cpp_lib_char8_t is defined. __cpp_char8_t enables only char8_t. This PR complements aa2e91f and 996328b.

More details:
https://developercommunity.visualstudio.com/t/support-for-cpp-char8-t-feature-test-macro-and-if/586040
https://en.cppreference.com/w/cpp/feature_test

In MSVS, when compiled with pre-c++20 compiler, char8_t is enabled with /Zc:char8_t flag. But this flag does not enable library support, which in MSVS comes only with c++20. It's quite easy to reproduce

PS C:\w\thirdparty\googletest\mybuild> git diff
diff --git a/googletest/cmake/internal_utils.cmake b/googletest/cmake/internal_utils.cmake
index 0438bef8..502f09f4 100644
--- a/googletest/cmake/internal_utils.cmake
+++ b/googletest/cmake/internal_utils.cmake
@@ -86,6 +86,7 @@ macro(config_compiler_and_linker)
     # Suppress "unreachable code" warning
     # http://stackoverflow.com/questions/3232669 explains the issue.
     set(cxx_base_flags "${cxx_base_flags} -wd4702")
+    set(cxx_base_flags "${cxx_base_flags} /Zc:char8_t")
     # Ensure MSVC treats source files as UTF-8 encoded.
     if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
       set(cxx_base_flags "${cxx_base_flags} -utf-8")

All tests passed:

[----------] Global test environment tear-down
[==========] 152 tests from 34 test suites ran. (2415 ms total)
[  PASSED  ] 152 tests.

Fixes: #4183

@higher-performance
Copy link
Collaborator

Thanks for the fix. Would you mind clarifying what version of Visual Studio you're using, and where we can obtain the installer? The page you linked to is concerning VS 16.1, which appears to be out of support and whose installer I seem to be unable to find. If you have any way to reproduce + verify this fix on a supported version of Visual Studio, please do let us know, otherwise I'm not sure what we can do here. Thanks!

Copy link
Contributor Author

@venik venik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@higher-performance I used 17.4 LTSC, but it's the same with clang on MacOS

I can't quickly figure out why cmake -Dgtest_build_tests=ON -Dgmock_build_tests=ON .. does not pick any flag from cxx_base_flags

alexnik-pvt-15:mybuild alexnik$ git diff
diff --git a/googletest/cmake/internal_utils.cmake b/googletest/cmake/internal_utils.cmake
index b09da33f..7c112c97 100644
--- a/googletest/cmake/internal_utils.cmake
+++ b/googletest/cmake/internal_utils.cmake
@@ -92,6 +92,7 @@ macro(config_compiler_and_linker)
     endif()
   elseif (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
     set(cxx_base_flags "-Wall -Wshadow -Wconversion -Wundef")
+    set(cxx_base_flags "${cxx_base_flags} -fchar8_t")
     set(cxx_exception_flags "-fexceptions")
     set(cxx_no_exception_flags "-fno-exceptions")
     set(cxx_strict_flags "-W -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wredundant-decls")

so I just added -fchar8_t to the empty string in generated CMakeCache.txt

alexnik-pvt-15:mybuild alexnik$ grep char8 CMakeCache.txt 
CMAKE_CXX_FLAGS:STRING=-fchar8_t

and result is pretty much the same

/Users/alexnik/study/googletest/googletest/include/gtest/gtest-printers.h:1014:29: error: no member named 'u8string' in namespace
      'std'
      UniversalPrint(::std::u8string(str), os);
                     ~~~~~~~^
In file included from /Users/alexnik/study/googletest/googletest/src/gtest-all.cc:46:
/Users/alexnik/study/googletest/googletest/src/gtest-printers.cc:532:35: error: no type named 'u8string' in namespace 'std'
void PrintU8StringTo(const ::std::u8string& s, ostream* os) {

previous commits did a very similar thing. __cpp_char8_t guards only char8_t, std implementation is guarded by __cpp_lib_char8_t and as for now, afaik, is not available on pre c++20 compilers.

alexnik-pvt-15:mybuild alexnik$ clang --version
Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin22.2.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@higher-performance
Copy link
Collaborator

Ah I see now, yeah, that looks right to me, thanks!

@copybara-service copybara-service bot merged commit 837f222 into google:main Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: MSVS: u8string is incorrectly guarded by __cpp_char8_t
2 participants