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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!



================
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
----------------
philnik wrote:
> You probably meant LLVM?
This is what we did in the past, I agree it's not entirely correct. For now I keep it as is. In the end this page is intended to be removed once format is complete.


================
Comment at: libcxx/include/print:146
+
+  // TODO FMT Validate with clang-tidy
+  // NOLINTNEXTLINE(bugprone-dangling-handle)
----------------
philnik wrote:
> 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.
Good point, mainly a force of habit to type FMT in format related places.
(Since print is quite small I prefer to track the status on the format page.)


================
Comment at: libcxx/include/print:194
+#  else
+  return isatty(fileno(__stream));
+#  endif
----------------
philnik wrote:
> Is this some sort of POSIX function? If yes, maybe we should check that we are on a POSIX system and error out otherwise?
Yes it's indeed POSIX and provided by `unistd`. I followed the method of `src/filesystem/operations.cpp` where we include this when not on Windows. Other places use `#if __has_include(<unistd.h>)`, so I switch to that method.


================
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");
----------------
philnik wrote:
> 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.
Good point!


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


================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/vprint_unicode_windows.pass.cpp:41
+
+TEST_GCC_DIAGNOSTIC_IGNORED("-Wuse-after-free")
+
----------------
philnik wrote:
> This should probably not be here, since this test is currently disabled for GCC.
I prefer to keep it since this was needed for GCC. I still hope we can enable format/print with GCC in the near future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150044