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

Remove /source-charset:utf-8 compile option. #2938

Merged
merged 1 commit into from
Jun 19, 2022

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Jun 13, 2022

Integrating fmt into my projects revealed the issue (https://godbolt.org/z/GqKEGexvo):

cl : Command line error D8016 : '/source-charset:utf-8' and '/utf-8' command-line options are incompatible

Compile options /utf-8, /source-charset:utf-8, /execution-charset:utf-8 are not compatible in any combination.
I'm using the globally set /utf-8 option, which results in an error when adding fmt with tests.

We can solve this problem globally by removing the '/source-charset:utf-8' option and replacing the string literal L"Файл.txt" with a wchar_t representation.

@vitaut, What do you think about this?

@fredemmott
Copy link

/utf-8 is /source-charset:utf-8 and /execution-charset:utf-8; perhaps specifying those separately for your project would work for you?

wchar_t's size (and UTF-16'ness) is Windows-specific; AIUI this approach isn't portable to the other platforms fmt supports

@ilyapopov
Copy link

wchar_t's size (and UTF-16'ness) is Windows-specific; AIUI this approach isn't portable to the other platforms fmt supports

This code is already guarded with #ifdef _WIN32

https://github.com/fmtlib/fmt/pull/2938/files#diff-d6fdd38fa0cc7a99925e7925eeae577c399747b4f8deb61a3aed03c0d56c007bR18

test/std-test.cc Outdated
@@ -16,7 +16,11 @@ TEST(std_test, path) {
"\"foo\\\"bar.txt\"");

# ifdef _WIN32
EXPECT_EQ(fmt::format("{}", std::filesystem::path(L"Файл.txt")),
// File.txt in Russian.
// Do not use utf-8 encoded chars in this file.
Copy link
Contributor

@vitaut vitaut Jun 14, 2022

Choose a reason for hiding this comment

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

This comment is a bit misleading because we actually use UTF-8 below. There is still a question whether it should be escaped or not but it's still UTF-8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment updated.

test/std-test.cc Outdated
// Do not use utf-8 encoded chars in this file.
const wchar_t cyrillic_path[] = {0x424, 0x430, 0x439, 0x43b, 0x2e,
0x74, 0x78, 0x74, 0};
EXPECT_EQ(fmt::format("{}", std::filesystem::path(cyrillic_path)),
"\"\xd0\xa4\xd0\xb0\xd0\xb9\xd0\xbb.txt\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use UTF-8 without escape sequences here as we do in format-test:

auto u = fmt::detail::utf8_to_utf16("лошадка");

test/std-test.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

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

In general looks good, a few comments inline.

@phprus
Copy link
Contributor Author

phprus commented Jun 14, 2022

@fredemmott,

fmt uses the /utf-8 flag in tests:

fmt/test/CMakeLists.txt

Lines 84 to 87 in fb991e9

add_fmt_test(unicode-test HEADER_ONLY)
if (MSVC)
target_compile_options(unicode-test PRIVATE /utf-8)
endif ()

Therefore, I cannot set the /source-charset:utf-8 /execution-charset:utf-8 option, it will conflict with /utf-8 from the tests (and several other third party libraries in my projects). And I cannot set the /utf-8 option, it will conflict with /source-charset:utf-8 from this test.

My mistake was choosing the /source-charset:utf-8 (PR ) option instead of using the wchar_t array.

wchar_t's size (and UTF-16'ness) is Windows-specific;
This test is only required for Windows. libstdc++ and libc++ use utf-8 in the std::filesystem::*.
This test will not reproduce the error when using the /utf-8 option or when there are utf-8 characters in the test source file.

@phprus phprus force-pushed the std-path-utf8-3 branch 3 times, most recently from 2dec732 to a8fca2d Compare June 18, 2022 09:48
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
@vitaut vitaut merged commit f0de128 into fmtlib:master Jun 19, 2022
@vitaut
Copy link
Contributor

vitaut commented Jun 19, 2022

Merged, thanks!

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.

4 participants