[libcxx-commits] [PATCH] D150044: [libc++][print] Adds FILE functions.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jun 27 23:09:42 PDT 2023
Mordante created this revision.
Herald added a subscriber: arichardson.
Herald added a project: All.
Mordante updated this revision to Diff 520106.
Mordante added a comment.
Mordante updated this revision to Diff 520109.
Mordante updated this revision to Diff 520116.
Mordante updated this revision to Diff 521900.
Mordante marked 3 inline comments as done.
Mordante updated this revision to Diff 521905.
Mordante marked an inline comment as done.
Mordante updated this revision to Diff 521906.
Mordante updated this revision to Diff 521989.
Mordante updated this revision to Diff 521996.
Mordante updated this revision to Diff 528101.
Mordante updated this revision to Diff 528133.
Mordante updated this revision to Diff 528136.
Mordante updated this revision to Diff 532213.
Mordante updated this revision to Diff 532251.
Mordante updated this revision to Diff 532375.
Mordante updated this revision to Diff 532381.
Mordante updated this revision to Diff 532461.
Herald added subscribers: mstorsjo, nemanjai.
Mordante updated this revision to Diff 532478.
Herald added a subscriber: emaste.
Mordante updated this revision to Diff 532707.
Mordante updated this revision to Diff 532886.
Mordante updated this revision to Diff 533336.
Mordante updated this revision to Diff 533348.
Mordante updated this revision to Diff 533472.
Mordante updated this revision to Diff 534196.
Mordante updated this revision to Diff 534199.
Mordante updated this revision to Diff 534204.
Mordante updated this revision to Diff 534207.
Mordante updated this revision to Diff 534214.
Mordante updated this revision to Diff 534236.
Mordante updated this revision to Diff 534247.
Mordante updated this revision to Diff 534330.
Mordante updated this revision to Diff 534349.
Mordante edited the summary of this revision.
Mordante updated this revision to Diff 534574.
Mordante updated this revision to Diff 535060.
Mordante published this revision for review.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.
CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
Partly addresses review comments.
Mordante added a comment.
CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
Several CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
Fixes a part of the CI issues.
Mordante added a comment.
Rebased and adjusted to upstream changes.
Mordante added a comment.
CI fixes
Mordante added a comment.
Rebased and updated modules to test with CI.
Mordante added a comment.
Rebased to fix CI.
Mordante added a comment.
CI fixes.
Mordante added a comment.
Update ABI list.
Mordante added a comment.
Fixes issues and test with reduced CI run.
Mordante added a comment.
Test CI and remove unwanted forwards.
Mordante added a comment.
Attempts to fix the CI.
Mordante added a comment.
Try to fix the CI.
Mordante added a comment.
Attempts to fix the CI.
Mordante added a comment.
Rebased and CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
CI fixes and update the lit flags.
Mordante added a comment.
CI fixes.
Mordante added a comment.
CI fixes.
Mordante added a comment.
Rebased, minimal improvement, and reanable full CI again.
Mordante added a comment.
Removes flush testing; its to unreliable.
Mordante added a comment.
CI fixes.
Mordante added a comment.
Trigger CI.
Mordante added a comment.
Did a final review before publishing.
Mordante added a comment.
Fixes a typo that broke the Windows build.
================
Comment at: libcxx/include/print:326
+_LIBCPP_HIDE_FROM_ABI void print(FILE* __stream, format_string<_Args...> __fmt, _Args&&... __args) {
+ // TODO FMT format doesn't forward to make_format_args, but print does.
+ if constexpr (__print::__use_unicode)
----------------
Remove this comment.
I spoke with Victor about this in the context of `P2905 R0 Runtime format strings` and the `std::forward` should indeed be removed
================
Comment at: libcxx/include/print:158-162
+// This is the same test MSVC STL uses in their implementation of
+// <print>
+// See: https://learn.microsoft.com/en-us/windows/win32/intl/code-page-identifiers
+inline constexpr bool __use_unicode =
+ _MSVC_EXECUTION_CHARACTER_SET == 65001 // Unicode (UTF-8) == 65001
----------------
Could we maybe nest these at the same level as the `#ifdef`? It took me sooo long to understand what this code was doing just due to formatting.
================
Comment at: libcxx/include/print:158-162
+// This is the same test MSVC STL uses in their implementation of
+// <print>
+// See: https://learn.microsoft.com/en-us/windows/win32/intl/code-page-identifiers
+inline constexpr bool __use_unicode =
+ _MSVC_EXECUTION_CHARACTER_SET == 65001 // Unicode (UTF-8) == 65001
----------------
ldionne wrote:
> Could we maybe nest these at the same level as the `#ifdef`? It took me sooo long to understand what this code was doing just due to formatting.
Actually the bad formatting here and below was due to a missing semi-colon after `_MSVC_EXECUTION_CHARACTER_SET == 65001`. Adding that fixed all formatting.
================
Comment at: libcxx/include/print:170-171
+
+ _LIBCPP_HIDE_FROM_ABI inline bool
+ __is_terminal(FILE * __stream) {
+# ifndef _WIN32
----------------
Same here, I think this should be unindented. It took me forever to be able to parse the `__is_terminal` declaration. If this is what clang-format did, I think it's a bug and we should use `// clang-format off` for this bit until the bug is fixed, cause as-is it's kinda cryptic.
================
Comment at: libcxx/src/print.cpp:26
+// Make this a weak function so tests can override it.
+void _LIBCPP_WEAK _LIBCPP_FUNC_VIS __WriteConsoleW(FILE* __steam, wstring_view __data) {
+ // https://learn.microsoft.com/en-us/windows/console/writeconsole
----------------
We could also just forward-declare `WriteConsoleW` on Windows in the headers without having to include `<windows.h>`. And then from our `__write_console` implementation detail function, we can call `_LIBCPP_TESTING_WRITE_CONSOLE_FUNCTION` if it is defined instead of calling `WriteConsoleW`. In the tests you `#define _LIBCPP_TESTING_WRITE_CONSOLE_FUNCTION ::my_write_console`. It's ugly but it would avoid introducing something in the dylib just for testing.
================
Comment at: libcxx/src/print.cpp:26
+// Make this a weak function so tests can override it.
+void _LIBCPP_WEAK _LIBCPP_FUNC_VIS __WriteConsoleW(FILE* __steam, wstring_view __data) {
+ // https://learn.microsoft.com/en-us/windows/console/writeconsole
----------------
ldionne wrote:
> We could also just forward-declare `WriteConsoleW` on Windows in the headers without having to include `<windows.h>`. And then from our `__write_console` implementation detail function, we can call `_LIBCPP_TESTING_WRITE_CONSOLE_FUNCTION` if it is defined instead of calling `WriteConsoleW`. In the tests you `#define _LIBCPP_TESTING_WRITE_CONSOLE_FUNCTION ::my_write_console`. It's ugly but it would avoid introducing something in the dylib just for testing.
I switched to the `_LIBCPP_TESTING_WRITE_CONSOLE_FUNCTION` but kept this as a non-weak function in the dylib.
`detail::make_windows_error` at line 30 is in the dylib too and I don't think we want to move that out of there.
================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp:16
+// Tests the implementation of
+// void __print::__vprint_unicode_posix(FILE* __stream, string_view __fmt,
+// format_args __args, bool __write_nl,
----------------
One thing we could do here instead is use the macro trick again and override the `__is_terminal` behavior by defining `_LIBCPP_TESTING_FORMAT_ASSUME_IS_TERMINAL` (or whatever name). This would allow writing this test using a standard API instead of using `__vprint_unicode_posix` directly. Again, it's not really pretty but it seems like there might not be a pretty way to test this.
================
Comment at: libcxx/utils/ci/run-buildbot:240
--exclude 'transcoding.pass.cpp' \
+ --exclude 'vprint_unicode_windows.pass.cpp' \
--exclude 'underflow.pass.cpp' \
----------------
Woops indentation
Drive-by fix to make sure the __retarget_buffer works correctly whan
using a hint of 1. This was discovered in one of the new tests.
Drive-by fixes __retarget_buffer when initialized with size 1.
Implements parts of
- P2093R14 Formatted output
- P2539R4 Should the output of std::print to a terminal be synchronized with the underlying stream?
Depends on D150031 <https://reviews.llvm.org/D150031>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D150044
Files:
libcxx/docs/FeatureTestMacroTable.rst
libcxx/docs/Status/Cxx23Papers.csv
libcxx/docs/Status/FormatIssues.csv
libcxx/docs/Status/FormatPaper.csv
libcxx/include/__format/buffer.h
libcxx/include/print
libcxx/include/version
libcxx/lib/abi/arm64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/powerpc-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/powerpc64-ibm-aix.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-apple-darwin.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-unknown-freebsd.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.nonew.abilist
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.noexceptions.nonew.abilist
libcxx/modules/std/print.cppm
libcxx/src/CMakeLists.txt
libcxx/src/print.cpp
libcxx/test/libcxx/input.output/iostream.format/print.fun/transcoding.pass.cpp
libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp
libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp
libcxx/test/libcxx/transitive_includes/cxx03.csv
libcxx/test/libcxx/transitive_includes/cxx11.csv
libcxx/test/libcxx/transitive_includes/cxx14.csv
libcxx/test/libcxx/transitive_includes/cxx17.csv
libcxx/test/libcxx/transitive_includes/cxx20.csv
libcxx/test/libcxx/transitive_includes/cxx23.csv
libcxx/test/libcxx/transitive_includes/cxx26.csv
libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp
libcxx/test/std/input.output/iostream.format/print.fun/print_tests.h
libcxx/test/std/input.output/iostream.format/print.fun/println.file.pass.cpp
libcxx/test/std/input.output/iostream.format/print.fun/vprint_nonunicode.file.pass.cpp
libcxx/test/std/input.output/iostream.format/print.fun/vprint_unicode.file.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/ostream.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/print.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
libcxx/test/std/utilities/format/format.functions/fill.unicode.pass.cpp
libcxx/utils/ci/run-buildbot
libcxx/utils/generate_feature_test_macro_components.py
libcxx/utils/libcxx/test/header_information.py
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D150044.535060.patch
Type: text/x-patch
Size: 82233 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230628/0dda43bc/attachment-0001.bin>
More information about the libcxx-commits
mailing list