The printf and fprintf implementations use our internal implementation
to improve performance when it's available, but this patch enables using
the public FILE API for overlay mode.
Details
- Reviewers
sivachandra lntue - Commits
- rGde939c6cd80c: [libc] enable printf using system FILE
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/src/stdio/printf_core/file_writer.cpp | ||
---|---|---|
2 | Can this #include be outside of the namespaces? | |
libc/test/src/stdio/CMakeLists.txt | ||
163 | @sivachandra : I would assume the current bazel build is kind of equivalent to overlay mode. So do we need to add this flag to the bazel overlay? |
libc/test/src/stdio/CMakeLists.txt | ||
---|---|---|
163 | the current bazel overlay doesn't have stdio yet, so this flag will be added after both this patch and the printf bazel patch (https://reviews.llvm.org/D146100) land. |
libc/src/stdio/CMakeLists.txt | ||
---|---|---|
521 | 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} ) | |
libc/src/stdio/printf.cpp | ||
14 | s/PUBLIC_FILE/SYSTEM_FILE | |
32–33 | 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. | |
libc/test/src/stdio/fprintf_test.cpp | ||
22 | s/file_test/printf_test |
libc/src/stdio/printf.cpp | ||
---|---|---|
32–33 | 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. |
libc/src/stdio/printf.cpp | ||
---|---|---|
32–33 | 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. | |
libc/src/stdio/printf_core/CMakeLists.txt | ||
127 | Even this if should be modified like the one in the other file. |
move vfprintf_internal and file_writer to templates
libc/src/stdio/printf.cpp | ||
---|---|---|
32–33 | I can't limit it to just this file, since fprintf also needs to select the correct template, but it's just in the entrypoints now. |
libc/src/stdio/fprintf.cpp | ||
---|---|---|
21 | Nit: Use UpperCase names for internal types. | |
libc/src/stdio/printf.cpp | ||
19 | Make this a macro, say PRINTF_STDOUT. Otherwise, you are adding a new symbol and an indirection. | |
libc/src/stdio/printf_core/file_writer.h | ||
89 | You can call flockfile in the constructor and funlockfile in the destructor, and make this a call to fwrite_unlocked. |
Use a more focused if: