Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[libc] Fix printf config not working #66834

Merged
merged 2 commits into from
Sep 19, 2023
Merged

[libc] Fix printf config not working #66834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3fe827
[libc] Fix printf config not working
michaelrj-google Sep 19, 2023
c74752b
Merge branch 'llvm:main' into libcPrintfCmakeOrderFix
michaelrj-google Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions libc/src/stdio/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ if(NOT LIBC_TARGET_ARCHITECTURE_IS_GPU)
add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/generic)
endif()

add_subdirectory(printf_core)
add_subdirectory(scanf_core)

add_entrypoint_object(
fflush
SRCS
Expand Down Expand Up @@ -286,6 +283,9 @@ add_entrypoint_object(
${printf_copts}
)

add_subdirectory(printf_core)
add_subdirectory(scanf_core)

add_entrypoint_object(
ftell
SRCS
Expand Down
12 changes: 12 additions & 0 deletions libc/test/src/stdio/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,16 @@ add_libc_unittest(
LibcMemoryHelpers
)

if(LIBC_CONF_PRINTF_DISABLE_FLOAT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this pattern is how the copts are to be used. In this particular case, I think the right way would be to split sprintf_test into sprintf_basic_test, sprintf_float_test, sprintf_index_mode_test, sprintf_write_int_test etc. Then, test targets should be added as follows:

add_libc_test(
  sprintf_basic_test
  ...
)

if(NOT LIBC_CONF_PRINTF_DISABLE_FLOAT)
  add_fp_unittest(
    sprintf_float_test
  )
endif()

# Other conditionals on config options

There could be some compile opts that need to be inherited by the tests [1]. Even in such a case, the test source code should not directly use those internal copts - they are internal implementation details unrelated to the tests.

[1] - We do not have such a mechanism set up currently. We can add it when required. It will be required if the tests are calling internal implementation functions and not the entrypoints. We do have such tests but they are typically testing common infrastructure components not affected by config options.

list(APPEND sprintf_test_copts "-DLIBC_COPT_PRINTF_DISABLE_FLOAT")
endif()
if(LIBC_CONF_PRINTF_DISABLE_INDEX_MODE)
list(APPEND sprintf_test_copts "-DLIBC_COPT_PRINTF_DISABLE_INDEX_MODE")
endif()
if(LIBC_CONF_PRINTF_DISABLE_WRITE_INT)
list(APPEND sprintf_test_copts "-DLIBC_COPT_PRINTF_DISABLE_WRITE_INT")
endif()

add_fp_unittest(
sprintf_test
UNIT_TEST_ONLY
Expand All @@ -123,6 +133,8 @@ add_fp_unittest(
libc.src.stdio.sprintf
libc.src.__support.FPUtil.fp_bits
libc.src.__support.FPUtil.platform_defs
COMPILE_OPTIONS
${sprintf_test_copts}
)

add_libc_unittest(
Expand Down