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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

It's really cool to see this coming together slowly. I didn't do any in-depth review since I'm not familiar with most of the code, but I have a few comments.



================
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
----------------
You probably meant LLVM?


================
Comment at: libcxx/include/print:146
+
+  // TODO FMT Validate with clang-tidy
+  // NOLINTNEXTLINE(bugprone-dangling-handle)
----------------
Would it make sense to use `TODO PRINT` or something like that instead? the actual output and formatting seem like two separate things to me.


================
Comment at: libcxx/include/print:194
+#  else
+  return isatty(fileno(__stream));
+#  endif
----------------
Is this some sort of POSIX function? If yes, maybe we should check that we are on a POSIX system and error out otherwise?


================
Comment at: libcxx/include/print:302
+#    else
+  // Note libc++ does not test Windows with wchar_t disabled.
+  std::__throw_format_error("Unable to convert UTF-8 to UTF-16 for the Windows Console");
----------------
I don't expect disabled wide characters and windows to be a common combination, so maybe just adding a compile time error would be the right thing to do here? That would probably be a lot easier to diagnose, and in the unlikely case that people complain we can still change it.


================
Comment at: libcxx/src/print.cpp:17
+#  include <io.h>
+#  include <windows.h>
+
----------------
Is lower or upper case the right thing here? I can never remember...


================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp:41
+
+TEST_GCC_DIAGNOSTIC_IGNORED("-Wuse-after-free")
+
----------------
This should probably not be here, since this test is currently disabled for GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150044