[libc-commits] [PATCH] D146001: [libc] enable printf using system FILE

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/stdio/printf.cpp:31
+#ifndef LIBC_COPT_PRINTF_USE_PUBLIC_FILE
   int ret_val = printf_core::vfprintf_internal(
       reinterpret_cast<::FILE *>(__llvm_libc::stdout), format, args);
----------------
michaelrj wrote:
> sivachandra wrote:
> > Have you considered making `vfprintf_internal` a template function which takes a suitable `FileWriter` object? The goal should be to confine `#ifdef` to only one place in the code. Since only one of the templates will get instantiated, it will not lead to code size bloat.
> `vfprintf_internal` already doesn't have this `#ifdef`, this condition only changes where the stdout comes from. I've changed the code to clarify that.
Sorry, my comment here is more general. Right now, you have a `#ifdef` conditional here, and two more in `file_writer.cpp` and `file_writer.h`. Using appropriate templates, you should be able to limit the `#ifdef` to just this file.


================
Comment at: libc/src/stdio/printf_core/CMakeLists.txt:123
+# implementation to improve performance.
+if(LLVM_LIBC_FULL_BUILD)
 add_object_library(
----------------
Even this `if` should be modified like the one in the other file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146001