From 01c46cc6f5fe1d6fe79be1143041304a4617a9c5 Mon Sep 17 00:00:00 2001 From: Sutou Kouhei Date: Thu, 4 Sep 2025 14:15:09 +0900 Subject: [PATCH] FYI: [C++] Use CMake target instead of add_definitions() for xxHash --- cpp/CMakeLists.txt | 6 ------ cpp/cmake_modules/ThirdpartyToolchain.cmake | 7 +++++++ cpp/src/arrow/CMakeLists.txt | 8 ++++++++ cpp/src/arrow/engine/CMakeLists.txt | 1 + cpp/src/arrow/util/CMakeLists.txt | 2 +- cpp/src/gandiva/CMakeLists.txt | 1 + cpp/src/parquet/CMakeLists.txt | 5 +++-- 7 files changed, 21 insertions(+), 9 deletions(-) diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index 4d466de521f80..5d3d58cb12cb1 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -351,12 +351,6 @@ endif() include(SetupCxxFlags) -if(${CMAKE_CXX_FLAGS_DEBUG} MATCHES "-Og") - # GH-47475: xxhash fails inlining when -Og is used. - # See: https://github.com/Cyan4973/xxHash/issues/943 - add_definitions(-DXXH_NO_INLINE_HINTS) -endif() - # # Linker flags # diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index acd7f42a0696c..045f48ee107c2 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -371,6 +371,13 @@ add_library(arrow::flatbuffers INTERFACE IMPORTED) target_include_directories(arrow::flatbuffers INTERFACE "${THIRDPARTY_DIR}/flatbuffers/include") +add_library(arrow::xxhash INTERFACE IMPORTED) +# GH-47475: xxhash fails inlining when -Og is +# used. -DCMAKE_BUILD_TYPE=Debug may use -Og. +# See: https://github.com/Cyan4973/xxHash/issues/943 +target_compile_definitions(arrow::xxhash + INTERFACE "$<$:XXH_NO_INLINE_HINTS>") + # ---------------------------------------------------------------------- # Some EP's require other EP's diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt index 0cc4765a79c6c..726ccabcdfa0f 100644 --- a/cpp/src/arrow/CMakeLists.txt +++ b/cpp/src/arrow/CMakeLists.txt @@ -402,6 +402,9 @@ arrow_add_object_library(ARROW_ARRAY array/statistics.cc array/util.cc array/validate.cc) +foreach(target ${ARROW_ARRAY_TARGETS}) + target_link_libraries(${target} PRIVATE arrow::xxhash) +endforeach() arrow_add_object_library(ARROW_IO io/buffered.cc @@ -865,6 +868,7 @@ if(ARROW_COMPUTE) ${ARROW_VERSION_SCRIPT_FLAGS} # Defined in cpp/arrow/CMakeLists.txt ) foreach(LIB_TARGET ${ARROW_COMPUTE_LIBRARIES}) + target_link_libraries(${LIB_TARGET} PRIVATE arrow::xxhash) target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_COMPUTE_EXPORTING) endforeach() @@ -874,6 +878,9 @@ if(ARROW_COMPUTE) endif() arrow_add_object_library(ARROW_COMPUTE_CORE ${ARROW_COMPUTE_SRCS}) +foreach(target ${ARROW_COMPUTE_CORE_TARGETS}) + target_link_libraries(${target} PRIVATE arrow::xxhash) +endforeach() if(ARROW_USE_XSIMD) foreach(ARROW_COMPUTE_CORE_TARGET ${ARROW_COMPUTE_CORE_TARGETS}) @@ -1167,6 +1174,7 @@ if(ARROW_BUILD_STATIC AND WIN32) endif() foreach(LIB_TARGET ${ARROW_LIBRARIES}) + target_link_libraries(${LIB_TARGET} PRIVATE arrow::xxhash) target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_EXPORTING) # C++17 is required to compile against Arrow C++ headers and libraries target_compile_features(${LIB_TARGET} PUBLIC cxx_std_17) diff --git a/cpp/src/arrow/engine/CMakeLists.txt b/cpp/src/arrow/engine/CMakeLists.txt index adf98087ad1d5..25047c7e9d99f 100644 --- a/cpp/src/arrow/engine/CMakeLists.txt +++ b/cpp/src/arrow/engine/CMakeLists.txt @@ -67,6 +67,7 @@ add_arrow_lib(arrow_substrait ${SUBSTRAIT_INCLUDES}) foreach(LIB_TARGET ${ARROW_SUBSTRAIT_LIBRARIES}) + target_link_libraries(${LIB_TARGET} PRIVATE arrow::xxhash) target_compile_definitions(${LIB_TARGET} PRIVATE ARROW_ENGINE_EXPORTING) endforeach() diff --git a/cpp/src/arrow/util/CMakeLists.txt b/cpp/src/arrow/util/CMakeLists.txt index df47389240ecc..f5f00a41ae080 100644 --- a/cpp/src/arrow/util/CMakeLists.txt +++ b/cpp/src/arrow/util/CMakeLists.txt @@ -38,7 +38,7 @@ else() set(IO_UTIL_TEST_SOURCES io_util_test.cc) endif() -set(ARROW_UTILITY_TEST_LINK_LIBS Boost::headers) +set(ARROW_UTILITY_TEST_LINK_LIBS Boost::headers arrow::xxhash) if(ARROW_USE_XSIMD) list(APPEND ARROW_UTILITY_TEST_LINK_LIBS ${ARROW_XSIMD}) endif() diff --git a/cpp/src/gandiva/CMakeLists.txt b/cpp/src/gandiva/CMakeLists.txt index 687e75f4b7370..c1e2e7b85c264 100644 --- a/cpp/src/gandiva/CMakeLists.txt +++ b/cpp/src/gandiva/CMakeLists.txt @@ -174,6 +174,7 @@ add_arrow_lib(gandiva LLVM::LLVM_LIBS) foreach(LIB_TARGET ${GANDIVA_LIBRARIES}) + target_link_libraries(${LIB_TARGET} PRIVATE arrow::xxhash) target_compile_definitions(${LIB_TARGET} PRIVATE GANDIVA_EXPORTING) endforeach() diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt index dc7d40d2a3866..704b334700b0e 100644 --- a/cpp/src/parquet/CMakeLists.txt +++ b/cpp/src/parquet/CMakeLists.txt @@ -309,7 +309,7 @@ add_arrow_lib(parquet if(WIN32 AND NOT (ARROW_TEST_LINKAGE STREQUAL "static")) add_library(parquet_test_support STATIC "${PARQUET_THRIFT_SOURCE_DIR}/parquet_types.cpp") - target_link_libraries(parquet_test_support thrift::thrift) + target_link_libraries(parquet_test_support PRIVATE thrift::thrift) list(PREPEND PARQUET_TEST_LINK_LIBS parquet_test_support) list(APPEND PARQUET_LIBRARIES parquet_test_support) endif() @@ -339,8 +339,9 @@ endif() add_definitions(-DPARQUET_THRIFT_VERSION_MAJOR=${Thrift_VERSION_MAJOR}) add_definitions(-DPARQUET_THRIFT_VERSION_MINOR=${Thrift_VERSION_MINOR}) -# Thrift requires these definitions for some types that we use foreach(LIB_TARGET ${PARQUET_LIBRARIES}) + target_link_libraries(${LIB_TARGET} PRIVATE arrow::xxhash) + # Thrift requires these definitions for some types that we use target_compile_definitions(${LIB_TARGET} PRIVATE PARQUET_EXPORTING PRIVATE HAVE_INTTYPES_H