[libcxx-commits] [libcxx] [libc++] Fix unnecessary fflush() in __vprint_unicode() on POSIX (PR #70321)
Dimitrij Mijoski via libcxx-commits
libcxx-commits at lists.llvm.org
Tue May 14 08:01:16 PDT 2024
https://github.com/dimztimz updated https://github.com/llvm/llvm-project/pull/70321
>From a0a17c1e6ae3cedd6b000aa52e68523af591bcd5 Mon Sep 17 00:00:00 2001
From: Dimitrij Mijoski <dmjpp at hotmail.com>
Date: Tue, 14 May 2024 13:02:50 +0200
Subject: [PATCH] [libc++] Fix unnecessary flushes in std::print() on POSIX
The check whether the stream is associated with a terminal or not is
needed only on Windows. The flush is needed only on Windows and when the
stream is terminal. The flushing is done only once before using the native
Unicode API on Windows.
Additionally, the correct flush is now called. In the case of C stream
overloads of print(), std::fflush() should be used, and in the
ostream overloads, only ostream::flush() member function should be used.
Before this fix, the ostream overloads called ostream::flush() and then
std::fflush().
See also https://wg21.link/LWG4044.
Fixes #70142
---
libcxx/include/__ostream/print.h | 8 +-
libcxx/include/print | 27 +++----
.../vprint_unicode.pass.cpp | 28 +++++++
.../print.fun/vprint_unicode_posix.pass.cpp | 77 -------------------
.../print.fun/vprint_unicode_windows.pass.cpp | 30 +-------
5 files changed, 43 insertions(+), 127 deletions(-)
delete mode 100644 libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp
diff --git a/libcxx/include/__ostream/print.h b/libcxx/include/__ostream/print.h
index 97680cdab6da3..a2e74a0bd2633 100644
--- a/libcxx/include/__ostream/print.h
+++ b/libcxx/include/__ostream/print.h
@@ -95,7 +95,7 @@ _LIBCPP_EXPORTED_FROM_ABI FILE* __get_ostream_file(ostream& __os);
# ifndef _LIBCPP_HAS_NO_UNICODE
template <class = void> // TODO PRINT template or availability markup fires too eagerly (http://llvm.org/PR61563).
_LIBCPP_HIDE_FROM_ABI void __vprint_unicode(ostream& __os, string_view __fmt, format_args __args, bool __write_nl) {
-# if _LIBCPP_AVAILABILITY_HAS_PRINT == 0
+# if _LIBCPP_AVAILABILITY_HAS_PRINT == 0 || !defined(_LIBCPP_WIN32API)
return std::__vprint_nonunicode(__os, __fmt, __args, __write_nl);
# else
FILE* __file = std::__get_ostream_file(__os);
@@ -118,10 +118,8 @@ _LIBCPP_HIDE_FROM_ABI void __vprint_unicode(ostream& __os, string_view __fmt, fo
# endif // _LIBCPP_HAS_NO_EXCEPTIONS
ostream::sentry __s(__os);
if (__s) {
-# ifndef _LIBCPP_WIN32API
- __print::__vprint_unicode_posix(__file, __fmt, __args, __write_nl, true);
-# elif !defined(_LIBCPP_HAS_NO_WIDE_CHARACTERS)
- __print::__vprint_unicode_windows(__file, __fmt, __args, __write_nl, true);
+# ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+ __print::__vprint_unicode_windows(__file, __fmt, __args, __write_nl);
# else
# error "Windows builds with wchar_t disabled are not supported."
# endif
diff --git a/libcxx/include/print b/libcxx/include/print
index e0bcf214ea239..d5eeca50513cd 100644
--- a/libcxx/include/print
+++ b/libcxx/include/print
@@ -234,26 +234,11 @@ __vprint_nonunicode(FILE* __stream, string_view __fmt, format_args __args, bool
// terminal when the output is redirected. Typically during testing the
// output is redirected to be able to capture it. This makes it hard to
// test this code path.
-template <class = void> // TODO PRINT template or availability markup fires too eagerly (http://llvm.org/PR61563).
-_LIBCPP_HIDE_FROM_ABI inline void
-__vprint_unicode_posix(FILE* __stream, string_view __fmt, format_args __args, bool __write_nl, bool __is_terminal) {
- // TODO PRINT Should flush errors throw too?
- if (__is_terminal)
- std::fflush(__stream);
-
- __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl);
-}
# ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
template <class = void> // TODO PRINT template or availability markup fires too eagerly (http://llvm.org/PR61563).
_LIBCPP_HIDE_FROM_ABI inline void
-__vprint_unicode_windows(FILE* __stream, string_view __fmt, format_args __args, bool __write_nl, bool __is_terminal) {
- if (!__is_terminal)
- return __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl);
-
- // TODO PRINT Should flush errors throw too?
- std::fflush(__stream);
-
+__vprint_unicode_windows([[maybe_unused]] FILE* __stream, string_view __fmt, format_args __args, bool __write_nl) {
string __str = std::vformat(__fmt, __args);
// UTF-16 uses the same number or less code units than UTF-8.
// However the size of the code unit is 16 bits instead of 8 bits.
@@ -314,9 +299,15 @@ __vprint_unicode([[maybe_unused]] FILE* __stream,
// Windows there is a different API. This API requires transcoding.
# ifndef _LIBCPP_WIN32API
- __print::__vprint_unicode_posix(__stream, __fmt, __args, __write_nl, __print::__is_terminal(__stream));
+ __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl);
# elif !defined(_LIBCPP_HAS_NO_WIDE_CHARACTERS)
- __print::__vprint_unicode_windows(__stream, __fmt, __args, __write_nl, __print::__is_terminal(__stream));
+ if (__print::__is_terminal(__stream)) {
+ // TODO PRINT Should flush errors throw too?
+ std::fflush(__stream);
+ __print::__vprint_unicode_windows(__stream, __fmt, __args, __write_nl);
+ } else {
+ __print::__vprint_nonunicode(__stream, __fmt, __args, __write_nl);
+ }
# else
# error "Windows builds with wchar_t disabled are not supported."
# endif
diff --git a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp
index 5438d31bca33f..f22e6ced50e93 100644
--- a/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp
+++ b/libcxx/test/libcxx/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp
@@ -74,14 +74,22 @@ static void test_is_terminal_file_stream() {
assert(stream.is_open());
assert(stream.good());
std::print(stream, "test");
+#ifdef _WIN32
assert(is_terminal_calls == 1);
+#else
+ assert(is_terminal_calls == 0);
+#endif
}
{
std::ofstream stream(filename);
assert(stream.is_open());
assert(stream.good());
std::print(stream, "test");
+#ifdef _WIN32
assert(is_terminal_calls == 2);
+#else
+ assert(is_terminal_calls == 0);
+#endif
}
}
@@ -98,7 +106,11 @@ static void test_is_terminal_rdbuf_derived_from_filebuf() {
std::ostream stream(&buf);
std::print(stream, "test");
+#ifdef _WIN32
assert(is_terminal_calls == 1);
+#else
+ assert(is_terminal_calls == 0);
+#endif
}
// When the stream is cout, clog, or cerr, its FILE* may be a terminal. Validate
@@ -108,15 +120,27 @@ static void test_is_terminal_std_cout_cerr_clog() {
is_terminal_result = false;
{
std::print(std::cout, "test");
+#ifdef _WIN32
assert(is_terminal_calls == 1);
+#else
+ assert(is_terminal_calls == 0);
+#endif
}
{
std::print(std::cerr, "test");
+#ifdef _WIN32
assert(is_terminal_calls == 2);
+#else
+ assert(is_terminal_calls == 0);
+#endif
}
{
std::print(std::clog, "test");
+#ifdef _WIN32
assert(is_terminal_calls == 3);
+#else
+ assert(is_terminal_calls == 0);
+#endif
}
}
@@ -149,7 +173,11 @@ static void test_is_terminal_is_flushed() {
// A terminal sync is called.
is_terminal_result = true;
std::print(stream, "");
+#ifdef _WIN32
assert(buf.sync_calls == 1); // only called from the destructor of the sentry
+#else
+ assert(buf.sync_calls == 0);
+#endif
}
int main(int, char**) {
diff --git a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp
deleted file mode 100644
index 9a50770d97dbc..0000000000000
--- a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp
+++ /dev/null
@@ -1,77 +0,0 @@
-//===----------------------------------------------------------------------===//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
-// UNSUPPORTED: no-filesystem
-// UNSUPPORTED: GCC-ALWAYS_INLINE-FIXME
-
-// XFAIL: availability-fp_to_chars-missing
-
-// fmemopen is available starting in Android M (API 23)
-// XFAIL: target={{.+}}-android{{(eabi)?(21|22)}}
-
-// REQUIRES: has-unix-headers
-
-// <print>
-
-// Tests the implementation of
-// void __print::__vprint_unicode_posix(FILE* __stream, string_view __fmt,
-// format_args __args, bool __write_nl,
-// bool __is_terminal);
-//
-// In the library when the stdout is redirected to a file it is no
-// longer considered a terminal and the special terminal handling is no
-// longer executed. By testing this function we can "force" emulate a
-// terminal.
-// Note __write_nl is tested by the public API.
-
-#include <algorithm>
-#include <array>
-#include <cassert>
-#include <cstdio>
-#include <print>
-
-#include "test_macros.h"
-
-int main(int, char**) {
- std::array<char, 100> buffer;
- std::ranges::fill(buffer, '*');
-
- FILE* file = fmemopen(buffer.data(), buffer.size(), "wb");
- assert(file);
-
- // Test the file is buffered.
- std::fprintf(file, "Hello");
- assert(std::ftell(file) == 5);
-#if defined(TEST_HAS_GLIBC) && \
- !(__has_feature(address_sanitizer) || __has_feature(thread_sanitizer) || __has_feature(memory_sanitizer))
- assert(std::ranges::all_of(buffer, [](char c) { return c == '*'; }));
-#endif
-
- // Test writing to a "non-terminal" stream does not flush.
- std::__print::__vprint_unicode_posix(file, " world", std::make_format_args(), false, false);
- assert(std::ftell(file) == 11);
-#if defined(TEST_HAS_GLIBC) && \
- !(__has_feature(address_sanitizer) || __has_feature(thread_sanitizer) || __has_feature(memory_sanitizer))
- assert(std::ranges::all_of(buffer, [](char c) { return c == '*'; }));
-#endif
-
- // Test writing to a "terminal" stream flushes before writing.
- std::__print::__vprint_unicode_posix(file, "!", std::make_format_args(), false, true);
- assert(std::ftell(file) == 12);
- assert(std::string_view(buffer.data(), buffer.data() + 11) == "Hello world");
-#if defined(TEST_HAS_GLIBC)
- // glibc does not flush after a write.
- assert(buffer[11] != '!');
-#endif
-
- // Test everything is written when closing the stream.
- std::fclose(file);
- assert(std::string_view(buffer.data(), buffer.data() + 12) == "Hello world!");
-
- return 0;
-}
diff --git a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp b/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp
index 2dd76052c2c97..dcb094e483115 100644
--- a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp
+++ b/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp
@@ -20,7 +20,7 @@
// Tests the implementation of
// void __print::__vprint_unicode_windows(FILE* __stream, string_view __fmt,
// format_args __args, bool __write_nl,
-// bool __is_terminal);
+// );
//
// In the library when the stdout is redirected to a file it is no
// longer considered a terminal and the special terminal handling is no
@@ -57,43 +57,19 @@ scoped_test_env env;
std::string filename = env.create_file("output.txt");
static void test_basics() {
- FILE* file = std::fopen(filename.c_str(), "wb");
- assert(file);
-
- // Test writing to a "non-terminal" stream does not call WriteConsoleW.
- std::__print::__vprint_unicode_windows(file, "Hello", std::make_format_args(), false, false);
- assert(std::ftell(file) == 5);
-
// It's not possible to reliably test whether writing to a "terminal" stream
// flushes before writing. Testing flushing a closed stream worked on some
// platforms, but was unreliable.
calling = true;
- std::__print::__vprint_unicode_windows(file, " world", std::make_format_args(), false, true);
+ std::__print::__vprint_unicode_windows(stdout, " world", std::make_format_args(), false);
}
// When the output is a file the data is written as-is.
// When the output is a "terminal" invalid UTF-8 input is flagged.
static void test(std::wstring_view output, std::string_view input) {
- // *** File ***
- FILE* file = std::fopen(filename.c_str(), "wb");
- assert(file);
-
- std::__print::__vprint_unicode_windows(file, input, std::make_format_args(), false, false);
- assert(std::ftell(file) == static_cast<long>(input.size()));
- std::fclose(file);
-
- file = std::fopen(filename.c_str(), "rb");
- assert(file);
-
- std::vector<char> buffer(input.size());
- size_t read = fread(buffer.data(), 1, buffer.size(), file);
- assert(read == input.size());
- assert(input == std::string_view(buffer.begin(), buffer.end()));
- std::fclose(file);
-
// *** Terminal ***
expected = output;
- std::__print::__vprint_unicode_windows(file, input, std::make_format_args(), false, true);
+ std::__print::__vprint_unicode_windows(stdout, input, std::make_format_args(), false);
}
static void test() {
More information about the libcxx-commits
mailing list