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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jul 23, 2022
Merged

Improve Unicode handling when writing to an ostream on Windows #2994

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0cbc004
Refactor detail::print() by splitting into two functions.
dimztimz Jul 20, 2022
e17de67
Make Unicode handing when writing to std::ostream more robust.
dimztimz Jul 18, 2022
e6d1489
Fix Unicode handling when writing to an ostream on GCC on Windows
dimztimz Jul 20, 2022
382f608
Add TODO note about detail::is_utf8()
dimztimz Jul 20, 2022
fe2e381
Fix warning -Wundef
dimztimz Jul 20, 2022
4230d74
Fix for non-Windows OSs
dimztimz Jul 20, 2022
7a94e60
Fix building as DLL on Windows
dimztimz Jul 20, 2022
5fd371e
Refactor
dimztimz Jul 23, 2022
72737d0
Suppress warning
dimztimz Jul 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions include/fmt/format-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1473,29 +1473,34 @@ FMT_FUNC std::string vformat(string_view fmt, format_args args) {
return to_string(buffer);
}

#ifdef _WIN32
namespace detail {
#ifdef _WIN32
using dword = conditional_t<sizeof(long) == 4, unsigned long, unsigned>;
extern "C" __declspec(dllimport) int __stdcall WriteConsoleW( //
void*, const void*, dword, dword*, void*);
} // namespace detail
#endif

namespace detail {
FMT_FUNC void print(std::FILE* f, string_view text) {
#ifdef _WIN32
FMT_FUNC bool write_console(std::FILE* f, string_view text) {
auto fd = _fileno(f);
if (_isatty(fd)) {
detail::utf8_to_utf16 u16(string_view(text.data(), text.size()));
auto written = detail::dword();
if (detail::WriteConsoleW(reinterpret_cast<void*>(_get_osfhandle(fd)),
u16.c_str(), static_cast<uint32_t>(u16.size()),
&written, nullptr)) {
return;
return true;
}
// Fallback to fwrite on failure. It can happen if the output has been
// redirected to NUL.
}
// We return false if the file descriptor was not TTY, or it was but
// SetConsoleW failed which can happen if the output has been redirected to
// NUL. In both cases when we return false, we should attempt to do regular
// write via fwrite or std::ostream::write.
return false;
}
#endif

FMT_FUNC void print(std::FILE* f, string_view text) {
#ifdef _WIN32
if (write_console(f, text)) return;
#endif
detail::fwrite_fully(text.data(), 1, text.size(), f);
}
Expand Down
3 changes: 3 additions & 0 deletions include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,6 +921,9 @@ struct is_contiguous<basic_memory_buffer<T, SIZE, Allocator>> : std::true_type {
};

namespace detail {
#ifdef _WIN32
FMT_API bool write_console(std::FILE* f, string_view text);
#endif
FMT_API void print(std::FILE*, string_view);
}

Expand Down
36 changes: 26 additions & 10 deletions include/fmt/ostream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@

#include <fstream>
#include <ostream>
#if defined(_WIN32) && defined(__GLIBCXX__)
# include <ext/stdio_filebuf.h>
# include <ext/stdio_sync_filebuf.h>
#endif
Comment on lines +13 to +16
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

@vitaut vitaut Jul 23, 2022 •
edited
Loading

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).

Copy link
Contributor Author

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

  1. https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_macros.html
  2. https://gcc.gnu.org/onlinedocs/libstdc++/manual/ext_io.html
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.


#include "format.h"

Expand Down Expand Up @@ -81,24 +85,36 @@ template class filebuf_access<filebuf_access_tag,
decltype(&filebuf_type::_Myfile),
&filebuf_type::_Myfile>;

inline bool write(std::filebuf& buf, fmt::string_view data) {
FILE* f = get_file(buf);
if (!f) return false;
print(f, data);
return true;
inline bool write_ostream_unicode(std::ostream& os, fmt::string_view data) {
#if FMT_MSC_VERSION
if (auto* buf = dynamic_cast<std::filebuf*>(os.rdbuf()))
if (FILE* f = get_file(*buf)) return write_console(f, data);
#elif defined(_WIN32) && defined(__GLIBCXX__)
auto* rdbuf = os.rdbuf();
FILE* c_file;
if (auto* fbuf = dynamic_cast<__gnu_cxx::stdio_sync_filebuf<char>*>(rdbuf))
c_file = fbuf->file();
else if (auto* fbuf = dynamic_cast<__gnu_cxx::stdio_filebuf<char>*>(rdbuf))
c_file = fbuf->file();
else
return false;
if (c_file) return write_console(c_file, data);
#else
(void)os; // suppress warning
(void)data;
dimztimz marked this conversation as resolved.
Show resolved Hide resolved
#endif
return false;
}
inline bool write(std::wfilebuf&, fmt::basic_string_view<wchar_t>) {
inline bool write_ostream_unicode(std::wostream&,
fmt::basic_string_view<wchar_t>) {
return false;
}

// Write the content of buf to os.
// It is a separate function rather than a part of vprint to simplify testing.
template <typename Char>
void write_buffer(std::basic_ostream<Char>& os, buffer<Char>& buf) {
if (const_check(FMT_MSC_VERSION)) {
auto filebuf = dynamic_cast<std::basic_filebuf<Char>*>(os.rdbuf());
if (filebuf && write(*filebuf, {buf.data(), buf.size()})) return;
}
if (write_ostream_unicode(os, {buf.data(), buf.size()})) return;
const Char* buf_data = buf.data();
using unsigned_streamsize = std::make_unsigned<std::streamsize>::type;
unsigned_streamsize size = buf.size();
Expand Down