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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review!



================
Comment at: libcxx/include/print:177
+
+#  ifndef _LIBCPP_HAS_NO_UNICODE
+#    ifdef _MSVC_EXECUTION_CHARACTER_SET
----------------
ldionne wrote:
> We could reorder like this instead, I think it would be easier to follow:
> 
> ```
> #ifdef _LIBCPP_HAS_NO_UNICODE
> inline constexpr bool __use_unicode = false;
> #elif defined(_MSVC_EXECUTION_CHARACTER_SET)
> inline constexpr bool __use_unicode = _MSVC_EXECUTION_CHARACTER_SET == 65001;
> #else
> inline constexpr bool __use_unicode = true;
> #endif
> ```
> 
> WDYT? This removes one layer.
Yes this looks better.


================
Comment at: libcxx/include/print:200
+
+template <class = void> // TODO PRINT template or std::to_chars(floating-point) availability markup fires too eagerly.
+_LIBCPP_HIDE_FROM_ABI inline void
----------------
ldionne wrote:
> Can you link to http://llvm.org/PR61563? Same thing below.
I slightly changed the message to include the report, I prefer to keep it at this location instead of before the template. Some functions have useful comment which I don't want to clutter with this todo.


================
Comment at: libcxx/test/libcxx/input.output/iostream.format/print.fun/transcoding.pass.cpp:39-42
+#ifdef TEST_HAS_GLIBC
   out = std::find_if(out, buffer.end(), [](CharT c) { return c != CharT('*'); });
   assert(out == buffer.end());
+#endif
----------------
ldionne wrote:
> I don't think this should be specific to Glibc?
Since this is not a part of this patch, I'll do the all_of change separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150044