-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve Unicode handling when writing to an ostream on Windows #2994
Conversation
The part with SetConsoleW is a separate function, without fwrite().
Calls to print(ostream&) in the special Unicode case on Windows fallback to writing via ostream::write instead of fwrite().
Can you enable the CI again, I pushed a fix. It was a compiler warning. |
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 for the PR.
#if defined(_WIN32) && defined(__GLIBCXX__) | ||
# include <ext/stdio_filebuf.h> | ||
# include <ext/stdio_sync_filebuf.h> | ||
#endif |
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 suggest checking include presence with FMT_HAS_INCLUDE
to make it more robust.
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.
Why? In case some future GCC removes these files? The checks for Windows and libstdc++ should be still there.
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 the files are remove the check will be broken in either case but the removal is easier to support with FMT_HAS_INCLUDE
. In general testing against features is more robust than testing against platforms.
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.
So I suggest something like
#if defined(_WIN32) && FMT_HAS_INCLUDE(<ext/stdio_filebuf.h>)
# include <ext/stdio_filebuf.h>
# include <ext/stdio_sync_filebuf.h>
# define FMT_USE_STDIO_FILEBUF
#endif
and later:
#ifdef FMT_USE_STDIO_FILEBUF
...
#endif
This will avoid duplicating checks and make the whole thing more robust (including to removal of headers).
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.
That will incur additional penalty during compilation when one does not use libstdc++ on Windows, e.g. in MSVC. There is additional filesystem access. The header ext/stdio_filebuf.h
is very much platform specific thing so it is better to check for platform. Also think of the scenario when someone under MSVC somehow puts ext/stdio_filebuf.h
. The check for the platform must be there, IMO.
See
- https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html
- https://gcc.gnu.org/onlinedocs/libstdc++/manual/ext_io.html
- https://gcc.gnu.org/onlinedocs/gcc-12.1.0/libstdc++/api/a01178.html
Beginning with 3.1, the extra basic_filebuf constructor and the fd() function were removed from the standard filebuf. Instead, <ext/stdio_filebuf.h> contains a derived class template called __gnu_cxx::stdio_filebuf. This class can be constructed from a C FILE* or a file descriptor, and provides the fd() function.
Its there since GCC v3.1 and will remain there because it is stable extension (not experimental). The older supported GCC compiler of FMT is v4.8.
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.
Fair enough.
include/fmt/ostream.h
Outdated
inline bool write_ostream_gcc_mingw_unicode(std::ostream& os, | ||
fmt::string_view data) { |
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.
Please merge this function into write_ostream_msvc_unicode
and keep the old name write
. This should reduce the scope of conditional compilation a bit and remove redundant copy for 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.
Done, but i used the name write_ostream_unicode
Merged, thanks. |
- Update from version 9.0.0 to 9.1.0 - Update of rootfile - Changelog 9.1.0 - 2022-08-27 * ``fmt::formatted_size`` now works at compile time `#3026 <https://github.com/fmtlib/fmt/pull/3026>`_ For example (`godbolt <https://godbolt.org/z/1MW5rMdf8>`__): .. code:: c++ #include <fmt/compile.h> int main() { using namespace fmt::literals; constexpr size_t n = fmt::formatted_size("{}"_cf, 42); fmt::print("{}\n", n); // prints 2 } * Fixed handling of invalid UTF-8 `#3038 <https://github.com/fmtlib/fmt/pull/3038>`_, `#3044 <https://github.com/fmtlib/fmt/pull/3044>`_, `#3056 <https://github.com/fmtlib/fmt/pull/3056>`_ * Improved Unicode support in ``ostream`` overloads of ``print`` `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_, `#3001 <https://github.com/fmtlib/fmt/pull/3001>`_, `#3025 <https://github.com/fmtlib/fmt/pull/3025>`_ * Fixed handling of the sign specifier in localized formatting on systems with 32-bit ``wchar_t`` `#3041 <https://github.com/fmtlib/fmt/issues/3041>`_). * Added support for wide streams to ``fmt::streamed`` `#2994 <https://github.com/fmtlib/fmt/pull/2994>`_ * Added the ``n`` specifier that disables the output of delimiters when formatting ranges `#2981 <https://github.com/fmtlib/fmt/pull/2981>`_, `#2983 <https://github.com/fmtlib/fmt/pull/2983>`_ For example (`godbolt <https://godbolt.org/z/roKqGdj8c>`__): .. code:: c++ #include <fmt/ranges.h> #include <vector> int main() { auto v = std::vector{1, 2, 3}; fmt::print("{:n}\n", v); // prints 1, 2, 3 } * Worked around problematic ``std::string_view`` constructors introduced in C++23 `#3030 <https://github.com/fmtlib/fmt/issues/3030>`_, `#3050 <https://github.com/fmtlib/fmt/issues/3050>`_ * Improve handling (exclusion) of recursive ranges `#2968 <https://github.com/fmtlib/fmt/issues/2968>`_, `#2974 <https://github.com/fmtlib/fmt/pull/2974>`_ * Improved error reporting in format string compilation `#3055 <https://github.com/fmtlib/fmt/issues/3055>`_ * Improved the implementation of `Dragonbox <https://github.com/jk-jeon/dragonbox>`_, the algorithm used for the default floating-point formatting `#2984 <https://github.com/fmtlib/fmt/pull/2984>`_ * Fixed issues with floating-point formatting on exotic platforms. * Improved the implementation of chrono formatting `#3010 <https://github.com/fmtlib/fmt/pull/3010>`_ * Improved documentation `#2966 <https://github.com/fmtlib/fmt/pull/2966>`_, `#3009 <https://github.com/fmtlib/fmt/pull/3009>`_, `#3020 <https://github.com/fmtlib/fmt/issues/3020>`_, `#3037 <https://github.com/fmtlib/fmt/pull/3037>`_ * Improved build configuration `#2991 <https://github.com/fmtlib/fmt/pull/2991>`_, `#2995 <https://github.com/fmtlib/fmt/pull/2995>`_, `#3004 <https://github.com/fmtlib/fmt/issues/3004>`_, `#3007 <https://github.com/fmtlib/fmt/pull/3007>`_, `#3040 <https://github.com/fmtlib/fmt/pull/3040>`_ * Fixed various warnings and compilation issues `#2969 <https://github.com/fmtlib/fmt/issues/2969>`_, `#2971 <https://github.com/fmtlib/fmt/pull/2971>`_, `#2975 <https://github.com/fmtlib/fmt/issues/2975>`_, `#2982 <https://github.com/fmtlib/fmt/pull/2982>`_, `#2985 <https://github.com/fmtlib/fmt/pull/2985>`_, `#2988 <https://github.com/fmtlib/fmt/issues/2988>`_, `#3000 <https://github.com/fmtlib/fmt/issues/3000>`_, `#3006 <https://github.com/fmtlib/fmt/issues/3006>`_, `#3014 <https://github.com/fmtlib/fmt/issues/3014>`_, `#3015 <https://github.com/fmtlib/fmt/issues/3015>`_, `#3021 <https://github.com/fmtlib/fmt/pull/3021>`_, `#3023 <https://github.com/fmtlib/fmt/issues/3023>`_, `#3024 <https://github.com/fmtlib/fmt/pull/3024>`_, `#3029 <https://github.com/fmtlib/fmt/pull/3029>`_, `#3043 <https://github.com/fmtlib/fmt/pull/3043>`_, `#3052 <https://github.com/fmtlib/fmt/issues/3052>`_, `#3053 <https://github.com/fmtlib/fmt/pull/3053>`_, `#3054 <https://github.com/fmtlib/fmt/pull/3054>`_ Signed-off-by: Adolf Belka <adolf.belka@ipfire.org> Reviewed-by: Michael Tremer <michael.tremer@ipfire.org>
See also #2875. I split the whole patch into 4 commits so reviewing is easier.