[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
Mon Jul 22 10:10:14 PDT 2024
https://github.com/dimztimz updated https://github.com/llvm/llvm-project/pull/70321
>From 65711434cff9d966f3bebad09994888d04a61f14 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 | 78 -------------------
.../print.fun/vprint_unicode_windows.pass.cpp | 30 +------
5 files changed, 43 insertions(+), 128 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 8265ac00777e2..464f6673121cb 100644
--- a/libcxx/include/__ostream/print.h
+++ b/libcxx/include/__ostream/print.h
@@ -94,7 +94,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);
@@ -117,10 +117,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 1a579daff270f..191d09ffe9059 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);
-}
# 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.
@@ -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 !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 9c801f5e044c1..62cbdc4f3a848 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
@@ -80,14 +80,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
}
}
@@ -104,7 +112,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
@@ -114,15 +126,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
}
}
@@ -155,7 +179,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 fd570c2c76619..0000000000000
--- a/libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp
+++ /dev/null
@@ -1,78 +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 7d17662da28fa..31a71dff90dbe 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
@@ -21,7 +21,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
@@ -58,43 +58,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 4c647e6671c7e5580060c3aee9f5dddfdb2637dd 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 62cbdc4f3a848..145d7944bdb97 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
@@ -18,7 +18,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 82e8f5e1b93c2fdaf79b93f3e0e2f4fa317c126a Mon Sep 17 00:00:00 2001
From: Dimitrij Mijoski <dmjpp at hotmail.com>
Date: Mon, 22 Jul 2024 19:07:14 +0200
Subject: [PATCH 3/3] fixup! [libc] Remove special case handing around test
case that was fixed
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 145d7944bdb97..47c273d9edabb 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
@@ -15,7 +15,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