-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[libc++] Fix unnecessary flushes in std::print() on POSIX #70321
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-libcxx Author: Dimitrij Mijoski (dimztimz) ChangesFixes #70142 Full diff: https://github.com/llvm/llvm-project/pull/70321.diff 2 Files Affected:
diff --git a/libcxx/include/print b/libcxx/include/print
index d119c8bda749768..29d417310e10582 100644
--- a/libcxx/include/print
+++ b/libcxx/include/print
@@ -230,15 +230,6 @@ __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).
@@ -310,7 +301,7 @@ __vprint_unicode([[maybe_unused]] FILE* __stream,
// Windows there is a different API. This API requires transcoding.
# ifndef _WIN32
- __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));
# else
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 9a50770d97dbcb2..000000000000000
--- 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;
-}
|
@vitaut would you mind taking a look too? |
The change looks correct to me. I haven't looked at the tests though. |
ad4db49
to
16de385
Compare
I updated this PR so it solves the bug for the ostream overloads too. The LWG issue is in state "tentatively ready", IMO good enough to accept this. |
82e8f5e
to
01d6932
Compare
I had a look and I think this is not a bug in libc++; it's a bug in the standard. |
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 llvm#70142
Fix test for modules
Fix for CI job Apple system configuration
01d6932
to
a52d81e
Compare
Fixes #70142