[libcxx-commits] [libcxx] [libc++] Fix unnecessary flushes in std::print() on POSIX (PR #70321)
Dimitrij Mijoski via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Nov 23 10:59:59 PST 2024
https://github.com/dimztimz updated https://github.com/llvm/llvm-project/pull/70321
>From 363e6f8ec658c840ee8e7906dd4fa83a8a07e2ee 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 1/3] [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 | 79 -------------------
.../print.fun/vprint_unicode_windows.pass.cpp | 30 +------
5 files changed, 43 insertions(+), 129 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 eb4233342214dd..f55497809d8b38 100644
--- a/libcxx/include/__ostream/print.h
+++ b/libcxx/include/__ostream/print.h
@@ -97,7 +97,7 @@ _LIBCPP_EXPORTED_FROM_ABI FILE* __get_ostream_file(ostream& __os);
# if _LIBCPP_HAS_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);
@@ -120,10 +120,8 @@ _LIBCPP_HIDE_FROM_ABI void __vprint_unicode(ostream& __os, string_view __fmt, fo
# endif // _LIBCPP_HAS_EXCEPTIONS
ostream::sentry __s(__os);
if (__s) {
-# ifndef _LIBCPP_WIN32API
- __print::__vprint_unicode_posix(__file, __fmt, __args, __write_nl, true);
-# elif _LIBCPP_HAS_WIDE_CHARACTERS
- __print::__vprint_unicode_windows(__file, __fmt, __args, __write_nl, true);
+# if _LIBCPP_HAS_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 8a8b686d18f34d..6856e33245c51c 100644
--- a/libcxx/include/print
+++ b/libcxx/include/print
@@ -233,26 +233,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);
-}
# if _LIBCPP_HAS_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.
@@ -313,9 +298,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 _LIBCPP_HAS_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 52d8500f7fa3a1..7882d237f8f20a 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
@@ -81,14 +81,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
}
}
@@ -105,7 +113,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
@@ -115,15 +127,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
}
}
@@ -156,7 +180,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 b89d02ba99425c..00000000000000
--- a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp
+++ /dev/null
@@ -1,79 +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: libcpp-has-no-unicode
-// 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 bcd1d05a3aeeb9..4777d9a3180f4e 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
@@ -22,7 +22,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
@@ -59,43 +59,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() {
>From f7a6a0ae6ed939b1253e8a189bf71a5b0d39ecbe Mon Sep 17 00:00:00 2001
From: Dimitrij Mijoski <dmjpp at hotmail.com>
Date: Tue, 14 May 2024 18:12:49 +0200
Subject: [PATCH 2/3] fixup! [libc++] Fix unnecessary flushes in std::print()
on POSIX
Fix test for modules
---
.../ostream.formatted.print/vprint_unicode.pass.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 7882d237f8f20a..c3c60ed705361d 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
@@ -19,7 +19,7 @@
// XFAIL: availability-print-missing
// Clang modules do not work with the definiton of _LIBCPP_TESTING_PRINT_IS_TERMINAL
-// XFAIL: clang-modules-build
+// XFAIL: clang-modules-build && target={{.*}}-windows{{.*}}
// <ostream>
// Tests the implementation of
>From a9d9b5c95d701badbde1279615ffd256aa0273ba Mon Sep 17 00:00:00 2001
From: Dimitrij Mijoski <dmjpp at hotmail.com>
Date: Tue, 23 Jul 2024 13:23:19 +0200
Subject: [PATCH 3/3] fixup! [libc++] Fix unnecessary flushes in std::print()
on POSIX
Fix for CI job Apple system configuration
---
.../ostream.formatted.print/vprint_unicode.pass.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 c3c60ed705361d..20bccf342f1657 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
@@ -16,7 +16,7 @@
// When std::print is unavailable, we don't rely on an implementation of
// std::__is_terminal and we always assume a non-unicode and non-terminal
// output.
-// XFAIL: availability-print-missing
+// XFAIL: availability-print-missing && target={{.*}}-windows{{.*}}
// Clang modules do not work with the definiton of _LIBCPP_TESTING_PRINT_IS_TERMINAL
// XFAIL: clang-modules-build && target={{.*}}-windows{{.*}}
More information about the libcxx-commits
mailing list