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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.
Herald added a subscriber: mikhail.ramalho.


================
Comment at: libc/src/stdio/CMakeLists.txt:509
+else()
+# In overlay mode, printf uses the system's stdout.
+add_entrypoint_object(
----------------
Use a more focused `if`:

```
list(APPEND printf_deps libc.src.__support.arg_list libc.src.stdio.printf_core.vfprintf_internal)
if(LLVM_LIBC_FULL_BUILD)
 list(APPEND printf_deps  libc.src.__support.File.file libc.src.__support.File.platform_file libc.src.__support.arg_list)
 set(printf_copts "COMPILE_OPTIONS -D<...>)
endif()

add_entrypoint_object(
  printf
  SRCS
    printf.cpp
  HDRS
    printf.h
  DEPENDS
    ${printf_deps}
  ${printf_copts}
)
```


================
Comment at: libc/src/stdio/printf.cpp:15
 
+#ifdef LIBC_COPT_PRINTF_USE_PUBLIC_FILE
+#include <stdio.h>
----------------
`s/PUBLIC_FILE/SYSTEM_FILE`


================
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);
----------------
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.


================
Comment at: libc/test/src/stdio/fprintf_test.cpp:22
 
+namespace file_test {
+#ifndef LIBC_COPT_PRINTF_USE_PUBLIC_FILE
----------------
`s/file_test/printf_test`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146001