[libcxx-commits] [PATCH] D156609: [libc++][print] Adds ostream overloads.

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/ostream:194-200
+#if _LIBCPP_STD_VER >= 23
+#  include <__format/format_args.h>
+#  include <__format/format_functions.h>
+#  include <cstdio>
+#  include <print>
+#  include <string_view>
+#endif
----------------
I'd rather not start conditionally including headers, since that will make the dependencies we have even more complicated.


================
Comment at: libcxx/include/ostream:1280-1282
+#    ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+  try {
+#    endif // _LIBCPP_HAS_NO_EXCEPTIONS
----------------
Maybe use an `__exception_guard` instead?


================
Comment at: libcxx/include/print:201
 _LIBCPP_HIDE_FROM_ABI inline bool __is_terminal(FILE* __stream) {
-#  ifdef _WIN32
+  // The macro _LIBCPP_TESTING_PRINT_IS_TERMINAL is used to change
+  // the behavior in the test. This is not part of the public API.
----------------
This seems like a really useful thing to make it an extension to me. E.g. clang has `-fcolor-diagnostics`, which is essentially what this function is doing.


================
Comment at: libcxx/src/ostream.cpp:20-31
+_LIBCPP_AVAILABILITY_PRINT _LIBCPP_EXPORTED_FROM_ABI FILE* __get_ostream_file(ostream& __os) {
+  auto* __rdbuf = __os.rdbuf();
+#ifndef _LIBCPP_HAS_NO_FILESYSTEM
+  if (auto* __buffer = dynamic_cast<filebuf*>(__rdbuf))
+    return __buffer->__file();
+#endif
+
----------------
No need to make this unreadable.


================
Comment at: libcxx/src/std_stream.h:115
 }
 
+inline bool __do_getc(FILE *__fp, char *__pbuf) {
----------------
I think the `inline` changes here make sense as their own NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156609