-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add an option to avoid wchar APIs on Windows #3636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I'm not a fan of supporting dead platforms but will be OK with merging this if the changes are more contained. In particular don't touch the CMake config and try to detect it automatically if possible.
@@ -18,7 +18,7 @@ | |||
# include <locale> | |||
#endif | |||
|
|||
#ifdef _WIN32 | |||
#if defined(_WIN32) && !defined(FMT_WINDOWS_NO_WCHAR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be detected automatically e.g. via some predefined macro or __has_include(<io.h>)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
It can't always be detected automatically but I've added the 2 cases where it can (missing <io.h>
on xbox, or defined WINVER
on properly configured Windows projects). Even when it can't be detected automatically, one can always add_definitions(FMT_WINDOWS_NO_WCHAR)
in the fmt subproject, so that's not a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping that it will be less messy but thanks for the effort =). It is particularly weird to have this tested in fmt/core.h
which explicitly avoids wchar_t
. Let's go back to requiring the user to define FMT_WINDOWS_NO_WCHAR
, just without the CMake config option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
88d2f8b
to
3045f5f
Compare
3f9ca52
to
461d0f7
Compare
include/fmt/core.h
Outdated
@@ -2675,7 +2675,7 @@ void vformat_to(buffer<Char>& buf, basic_string_view<Char> fmt, | |||
typename vformat_args<Char>::type args, locale_ref loc = {}); | |||
|
|||
FMT_API void vprint_mojibake(std::FILE*, string_view, format_args); | |||
#ifndef _WIN32 | |||
#if !defined(_WIN32) || defined(FMT_WINDOWS_NO_WCHAR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not change this? It's weird to disable overload that doesn't even use wchar_t
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have wchar, we don't have vprint_mojibake
either, and the compiler complains about the lack of a definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually maybe I'm wrong, let me investigate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only happens when is_utf8()
returns false
but here is the error:
fmt/core.h:2894: undefined reference to `fmt::v10::detail::vprint_mojibake(_iobuf*, fmt::v10::basic_string_view<char>, fmt::v10::basic_format_args<fmt::v10::basic_format_context<fmt::v10::appender, char> >)'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a wrong fix though because this will just discard the output. The correct fix is to enable the vprint_mojibake
definition in format-inl.h
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done
With this, fmt can be used on Windows 98 and the Original Xbox with: set(FMT_OS OFF) It is not exposed as a CMake option but one can define it manually in the fmt subproject, e.g.: target_compile_definitions(fmt PUBLIC FMT_WINDOWS_NO_WCHAR) Fixes fmtlib#3631
With this, fmt can be used on Windows 98 and the Original Xbox with: set(FMT_OS OFF) It is not exposed as a CMake option but one can define it manually in the fmt subproject, e.g.: target_compile_definitions(fmt PUBLIC FMT_WINDOWS_NO_WCHAR) Fixes fmtlib#3631
With this, fmt can be used on Windows 98 and the Original Xbox with:
Fixes #3631