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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/ostream:154
 template<class traits>
 basic_ostream<char, traits>& operator<<(basic_ostream<char, traits>&, const char32_t*) = delete;       // since C++20
 template<class traits>
----------------
General question: do we follow this recommended practice? https://eel.is/c++draft/ostream.formatted.print#4

> Recommended practice: For vprint_unicode, if invoking the native Unicode API requires transcoding, implementations should substitute invalid code units with U+FFFD REPLACEMENT CHARACTER per the Unicode Standard, Chapter 3.9 U+FFFD Substitution in Conversion.


================
Comment at: libcxx/include/ostream:1253
+// Returns the FILE* associated with the __os.
+// Returns a nullptr when no FILE* is accociated with __os.
+// This function is in the dylib since the type of the buffer associated
----------------



================
Comment at: libcxx/include/ostream:1275
+
+  __os.flush();
+
----------------
Given how long we discussed this line during live review, it might be worth explaining why this flush is correct.


================
Comment at: libcxx/include/print:203
+  // the behavior in the test. This is not part of the public API.
+#  ifdef _LIBCPP_TESTING_PRINT_IS_TERMINAL
+  return _LIBCPP_TESTING_PRINT_IS_TERMINAL(__stream);
----------------
Can you explain why this macro is necessary? How did you handle testing the previous `print` code without having to introduce this macro?


================
Comment at: libcxx/src/ostream.cpp:23
+#ifndef _LIBCPP_HAS_NO_FILESYSTEM
+  if (auto* __buffer = dynamic_cast<filebuf*>(__rdbuf))
+    return __buffer->__file();
----------------
We should restore the `-fno-rtti` CI configuration that we seem to have dropped at some point, and then we can reconsider whether this approach is feasible: https://godbolt.org/z/WzEEz3TP1

IDK if this will be possible to implement without RTTI, since we can't add a new virtual function to `basic_streambuf`.


================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp:91
+//   regard to the value of os.exceptions() and without turning on
+//   ios_base​::​badbit in the error state of os.
+// Most invalid format strings are checked at compile-time. An invalid
----------------
Weird characters here!


================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.formatted.print/vprint_unicode.pass.cpp:182
+  sstr.width(5);
+  test("+\U0010FFFF", "{}", "\U0010FFFF"); // Undefined Character
+}
----------------
Can you link to the recommended practice here? https://eel.is/c++draft/ostream.formatted.print#4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156609