[GitHub] [arrow] cyb70289 commented on a diff in pull request #13244: ARROW-12626: [C++] Support toolchain xsimd, update toolchain version to version 8.1.0

[GitBox <gi...@apache.org>]

cyb70289 commented on code in PR #13244:
URL: https://github.com/apache/arrow/pull/13244#discussion_r889692916


##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2234,16 +2234,22 @@ if((NOT ARROW_SIMD_LEVEL STREQUAL "NONE") OR (NOT ARROW_RUNTIME_SIMD_LEVEL STREQ
 else()
   set(ARROW_USE_XSIMD FALSE)
 endif()
+
+set(ARROW_XSIMD_REQUIRED_VERSION "8.1.0")
+
 if(ARROW_USE_XSIMD)
-  set(xsimd_SOURCE "BUNDLED")
-  resolve_dependency(xsimd)
+  resolve_dependency(xsimd REQUIRED_VERSION ${ARROW_XSIMD_REQUIRED_VERSION})
 
-  add_library(xsimd INTERFACE IMPORTED)
-  if(CMAKE_VERSION VERSION_LESS 3.11)
-    set_target_properties(xsimd PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
-                                           "${XSIMD_INCLUDE_DIR}")
+  if(xsimd_SOURCE STREQUAL "BUNDLED")
+    add_library(xsimd INTERFACE IMPORTED)
+    if(CMAKE_VERSION VERSION_LESS 3.11)
+      set_target_properties(xsimd PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
+                                             "${XSIMD_INCLUDE_DIR}")
+    else()
+      target_include_directories(xsimd INTERFACE "${XSIMD_INCLUDE_DIR}")
+    endif()

Review Comment:
   Curious why need to check version and call two different functions.
   `target_include_directories` is not supported before cmake 3.11?
   Can we use `set_target_properties` on all version?
   
   Some code doesn't check version and only calls `target_include_directores`. Is it a problem?
   e.g. https://github.com/apache/arrow/blob/master/cpp/cmake_modules/ThirdpartyToolchain.cmake#L4631



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org