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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
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
----------------
philnik wrote:
> I'd rather not start conditionally including headers, since that will make the dependencies we have even more complicated.
Good point, that was part of a test and should have been unconditionally.


================
Comment at: libcxx/include/ostream:1280-1282
+#    ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+  try {
+#    endif // _LIBCPP_HAS_NO_EXCEPTIONS
----------------
philnik wrote:
> Maybe use an `__exception_guard` instead?
I prefer to keep it as is now, this is consistent with the existing code, for example line 1228. Then make that change in one patch for the whole file, `<ios>` and `<iomanip>`


================
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.
----------------
philnik wrote:
> 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.
I'm not entirely sure what your suggestion is. Could you elaborate?


================
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
+
----------------
philnik wrote:
> No need to make this unreadable.
We use uglified code in other places in the dylib too. I prefer to keep it as is. When the code can be more from the dylib to headers there's no need to rename things.


================
Comment at: libcxx/src/std_stream.h:115
 }
 
+inline bool __do_getc(FILE *__fp, char *__pbuf) {
----------------
philnik wrote:
> I think the `inline` changes here make sense as their own NFC patch.
Agreed, I think it makes sense to just commit this when the CI is happy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156609