[libcxx-commits] [PATCH] D150044: [libc++][print] Adds FILE functions.

Tue Jul 11 10:01:34 PDT 2023

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Nice, thanks! There's a lot of details in this patch, but it means that users would have a really hard time doing this right if they did it themselves!

================
Comment at: libcxx/docs/Status/FormatPaper.csv:51
 `[print.fun] <https://wg21.link/print.fun>`__,"Output to ``stdout``",,Mark de Wever,|In Progress|,
-`[print.fun] <https://wg21.link/print.fun>`__,"Output to ``FILE*``",,Mark de Wever,,
+`[print.fun] <https://wg21.link/print.fun>`__,"Output to ``FILE*``",,Mark de Wever,|Complete|, Clang 17
 `[ostream.formatted.print] <https://wg21.link/ostream.formatted.print>`__,"Output to ``ostream``",,Mark de Wever
----------------
I think this should be `17.0`, and this patch needs to be rebased.

================
Comment at: libcxx/include/print:67
+// The function does not depend on the language standard used. Guarding
+// it with C++23 would fail since the dylib is currently build using C++20.
+//
----------------

================
Comment at: libcxx/include/print:97-102
+// Clang-format 16 aligns the '=' below with the '<' above.
+// TODO(LLVM-17) Enable clang-format
+// clang-format off
 template <class _Tp>
 concept __utf32_code_unit = same_as<_Tp, char32_t> _LIBCPP_IF_WIDE_CHARACTERS(|| same_as<_Tp, wchar_t>);
+// clang-format on
----------------
I don't think this hunk should be in this patch.

================
Comment at: libcxx/include/print:177
+
+#  ifndef _LIBCPP_HAS_NO_UNICODE
+#    ifdef _MSVC_EXECUTION_CHARACTER_SET
----------------
We could reorder like this instead, I think it would be easier to follow:

```
#ifdef _LIBCPP_HAS_NO_UNICODE
inline constexpr bool __use_unicode = false;
#elif defined(_MSVC_EXECUTION_CHARACTER_SET)
inline constexpr bool __use_unicode = _MSVC_EXECUTION_CHARACTER_SET == 65001;
#else
inline constexpr bool __use_unicode = true;
#endif
```

WDYT? This removes one layer.

================
Comment at: libcxx/include/print:179-180
+#    ifdef _MSVC_EXECUTION_CHARACTER_SET
+// 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
----------------

================
Comment at: libcxx/include/print:200
+
+template <class = void> // TODO PRINT template or std::to_chars(floating-point) availability markup fires too eagerly.
+_LIBCPP_HIDE_FROM_ABI inline void
----------------
Can you link to http://llvm.org/PR61563? Same thing below.

================
Comment at: libcxx/include/print:203
+__vprint_nonunicode(FILE* __stream, string_view __fmt, format_args __args, bool __write_nl) {
+  _LIBCPP_ASSERT(__stream, "__stream is a valid pointer to an output C stream");
+  string __str = std::vformat(__fmt, __args);
----------------
Let's use `_LIBCPP_UNCATEGORIZED_ASSERT` everywhere for now.

================
Comment at: libcxx/include/print:221
+// terminal when the output is redirected. Typically during testing the
+// output is redirected to be able to capture it. This makes is hard to
+// test this code path.
----------------

================
Comment at: libcxx/include/print:247
+  //
+  // The buffer uses the worse-case estimate and should never resize.
+  // However when the string is large this could lead to OOM. Using a
----------------

================
Comment at: libcxx/include/print:304
+#    else
+#      error "Windows builds with wchar_t disable are not supported."
+#    endif
----------------

================
Comment at: libcxx/include/print:337-340
+template <class = void> // TODO PRINT template or std::to_chars(floating-point) availability markup fires too eagerly.
+_LIBCPP_HIDE_FROM_ABI inline void vprint_unicode(FILE* __stream, string_view __fmt, format_args __args) {
+  __print::__vprint_unicode(__stream, __fmt, __args, false);
+}
----------------
Suggestions:

1. Don't define this function when unicode has been disabled. It's less surprising to have a compilation error when using this function on a system where unicode has been disabled than to call it and get non-unicode output.
2. Don't define `__vprint_unicode` (above) when unicode is disabled.
3. Switch the `if constexpr (__print::__use_unicode)` above in `println` and `print` to `#if <USE UNICODE>`

================
Comment at: libcxx/src/print.cpp:43
+#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
+// Make this a weak function so tests can override it.
+_LIBCPP_EXPORTED_FROM_ABI void
----------------
I think this comment is outdated.

================
Comment at: libcxx/src/print.cpp:45
+_LIBCPP_EXPORTED_FROM_ABI void
+__write_to_windows_console([[maybe_unused]] FILE* __stream, [[maybe_unused]] wstring_view __view) {
+#  if defined(_LIBCPP_WIN32API)
----------------
I don't think this function is needed outside of Windows anymore, since you now use a macro for testing. This would also get rid of the diff in the ABI list.

================
Comment at: libcxx/src/print.cpp:54
+#    ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+    throw system_error{filesystem::detail::make_windows_error(GetLastError()), "failed to write formatted output"};
+#    else
----------------
`__throw_system_error`? This way you don't need `ifndef _LIBCPP_HAS_NO_EXCEPTIONS`.

================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/transcoding.pass.cpp:39-42
+#ifdef TEST_HAS_GLIBC
   out = std::find_if(out, buffer.end(), [](CharT c) { return c != CharT('*'); });
   assert(out == buffer.end());
+#endif
----------------
I don't think this should be specific to Glibc?

================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp:44
+  // Test the file is buffered.
+  fprintf(file, "Hello");
+  assert(std::ftell(file) == 5);
----------------

================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_posix.pass.cpp:48
+    !(__has_feature(address_sanitizer) || __has_feature(thread_sanitizer) || __has_feature(memory_sanitizer))
+  assert(std::ranges::find_if(buffer, [](char c) { return c != '*'; }) == buffer.end());
+#endif
----------------
Maybe we can use `ranges::all_of` here too?

================
Comment at: libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp:67
+
+#if defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 200809L
+// The FILE returned by fmemopen does not have file descriptor.
----------------
We're not using `_POSIX_C_SOURCE` anywhere in the code base right now, and it looks like at least macOS doesn't define it properly. I generally try to steer away from checking specific macros in the test suite if I can because it's easy to disable tests without intending it.

Instead, I would suggest moving this test to another `.pass.cpp` file and we can mark it as `XFAIL` or `UNSUPPORTED` on the relevant platforms.

================
Comment at: libcxx/test/std/input.output/iostream.format/print.fun/print.file.pass.cpp:70
+// This means the test could fail when the implementation uses a
+// function that requires a file decriptor, for example write.
+static void test_no_file_descriptor() {
----------------

================
Comment at: libcxx/test/std/input.output/iostream.format/print.fun/print_tests.h:67
+  check_exception("Argument index out of bounds", "hello {0}");
+  check_exception("Argument index out of bounds", "hello {1}", 42);
+}
----------------
Can you add some unicode test cases here? It should just write the data as-is IIUC.

================
Comment at: libcxx/utils/libcxx/test/header_information.py:43
     "iostream": "// UNSUPPORTED: no-localization",
+    "print": "// UNSUPPORTED: availability-fp_to_chars-missing", # TODO FMT investigate
     "istream": "// UNSUPPORTED: no-localization",
----------------
Can you keep this list sorted?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150044/new/

https://reviews.llvm.org/D150044