gemini-code-assist[bot] commented on code in PR #351: URL: https://github.com/apache/tvm-ffi/pull/351#discussion_r2636514889
########## cmake/Utils/Library.cmake: ########## @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +include(CMakeParseArguments) Review Comment:  The `cmake_parse_arguments` function is built-in since CMake 3.5. Given that the example `CMakeLists.txt` requires version 3.18, this `include(CMakeParseArguments)` is likely redundant. If the minimum required CMake version for projects using this file is 3.5 or higher, you can remove this line. ########## cmake/Utils/Library.cmake: ########## @@ -137,3 +138,185 @@ function (tvm_ffi_add_target_from_obj target_name obj_target_name) tvm_ffi_add_apple_dsymutil(${target_name}_shared) tvm_ffi_add_apple_dsymutil(${target_name}_testing) endfunction () + +# cmake-lint: disable=C0301,R0912,R0915 +# ~~~ +# tvm_ffi_add(target_name [LINK_SHARED ON|OFF] [LINK_HEADER ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF]) +# Configure a target to integrate with TVM-FFI CMake utilities: +# - Optionally link against tvm_ffi_header and/or tvm_ffi_shared +# - Always apply tvm_ffi_add_prefix_map(target_name <current source dir>) +# - Optionally enable Apple dSYM generation via tvm_ffi_add_apple_dsymutil(target_name) +# - Optionally apply MSVC-specific flags via tvm_ffi_add_msvc_flags(target_name) +# +# Parameters: +# target_name: Existing CMake target to modify (positional, required) +# +# Keyword parameters (all accept ON/OFF-like values; defaults shown): +# LINK_SHARED: Whether to link tvm_ffi_shared into the target (default: ON) +# LINK_HEADER: Whether to link tvm_ffi_header into the target (default: ON) +# DEBUG_SYMBOL: Whether to enable debug symbol post-processing hooks. +# On Apple this calls tvm_ffi_add_apple_dsymutil(target_name) (default: ON) +# On non-Apple platforms this is currently a no-op unless you extend it. (default: ON) +# MSVC_FLAGS: Whether to call tvm_ffi_add_msvc_flags(target_name) to apply MSVC-specific flags (default: ON) +# +# Notes: +# - Installation is intentionally split out. Use tvm_ffi_install(target_name DESTINATION <dir>) to install artifacts. +# - This function requires tvm_ffi_add_prefix_map() to be defined/included. +# - If LINK_SHARED/LINK_HEADER are ON, the corresponding targets (tvm_ffi_shared/tvm_ffi_header) must exist. +# ~~~ +function (tvm_ffi_add tvm_ffi_cmake_arg_target) + if (NOT tvm_ffi_cmake_arg_target) + message( + FATAL_ERROR + "tvm_ffi_add: missing target name. " + "Usage: tvm_ffi_add(<target> [LINK_SHARED ON|OFF] [LINK_HEADER ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF])" + ) + endif () + + if (NOT TARGET "${tvm_ffi_cmake_arg_target}") + message( + FATAL_ERROR "tvm_ffi_add: '${tvm_ffi_cmake_arg_target}' is not an existing CMake target." + ) + endif () + + # Parse keyword args after the positional target name. + set(tvm_ffi_cmake_arg_options) # none; require explicit ON/OFF style values + set(tvm_ffi_cmake_arg_oneValueArgs LINK_SHARED LINK_HEADER DEBUG_SYMBOL MSVC_FLAGS) + set(tvm_ffi_cmake_arg_multiValueArgs) + + cmake_parse_arguments( + tvm_ffi_cmake_arg_ "${tvm_ffi_cmake_arg_options}" "${tvm_ffi_cmake_arg_oneValueArgs}" + "${tvm_ffi_cmake_arg_multiValueArgs}" ${ARGN} + ) + + # Defaults + if (NOT DEFINED tvm_ffi_cmake_arg__LINK_SHARED) + set(tvm_ffi_cmake_arg__LINK_SHARED ON) + endif () + if (NOT DEFINED tvm_ffi_cmake_arg__LINK_HEADER) + set(tvm_ffi_cmake_arg__LINK_HEADER ON) + endif () + if (NOT DEFINED tvm_ffi_cmake_arg__DEBUG_SYMBOL) + set(tvm_ffi_cmake_arg__DEBUG_SYMBOL ON) + endif () + if (NOT DEFINED tvm_ffi_cmake_arg__MSVC_FLAGS) + set(tvm_ffi_cmake_arg__MSVC_FLAGS ON) + endif () Review Comment:  This block of code for setting default values is repetitive. You can make it more concise and easier to maintain by using a `foreach` loop. Note that this suggestion uses the current variable prefix; if you change it as suggested in my other comment, you'll need to adjust this loop accordingly. ``` foreach(arg IN ITEMS LINK_SHARED LINK_HEADER DEBUG_SYMBOL MSVC_FLAGS) if(NOT DEFINED tvm_ffi_cmake_arg__${arg}) set(tvm_ffi_cmake_arg__${arg} ON) endif() endforeach() ``` ########## cmake/Utils/Library.cmake: ########## @@ -137,3 +138,185 @@ function (tvm_ffi_add_target_from_obj target_name obj_target_name) tvm_ffi_add_apple_dsymutil(${target_name}_shared) tvm_ffi_add_apple_dsymutil(${target_name}_testing) endfunction () + +# cmake-lint: disable=C0301,R0912,R0915 +# ~~~ +# tvm_ffi_add(target_name [LINK_SHARED ON|OFF] [LINK_HEADER ON|OFF] [DEBUG_SYMBOL ON|OFF] [MSVC_FLAGS ON|OFF]) +# Configure a target to integrate with TVM-FFI CMake utilities: +# - Optionally link against tvm_ffi_header and/or tvm_ffi_shared +# - Always apply tvm_ffi_add_prefix_map(target_name <current source dir>) +# - Optionally enable Apple dSYM generation via tvm_ffi_add_apple_dsymutil(target_name) +# - Optionally apply MSVC-specific flags via tvm_ffi_add_msvc_flags(target_name) +# +# Parameters: +# target_name: Existing CMake target to modify (positional, required) +# +# Keyword parameters (all accept ON/OFF-like values; defaults shown): +# LINK_SHARED: Whether to link tvm_ffi_shared into the target (default: ON) +# LINK_HEADER: Whether to link tvm_ffi_header into the target (default: ON) +# DEBUG_SYMBOL: Whether to enable debug symbol post-processing hooks. +# On Apple this calls tvm_ffi_add_apple_dsymutil(target_name) (default: ON) +# On non-Apple platforms this is currently a no-op unless you extend it. (default: ON) +# MSVC_FLAGS: Whether to call tvm_ffi_add_msvc_flags(target_name) to apply MSVC-specific flags (default: ON) +# +# Notes: +# - Installation is intentionally split out. Use tvm_ffi_install(target_name DESTINATION <dir>) to install artifacts. +# - This function requires tvm_ffi_add_prefix_map() to be defined/included. +# - If LINK_SHARED/LINK_HEADER are ON, the corresponding targets (tvm_ffi_shared/tvm_ffi_header) must exist. +# ~~~ +function (tvm_ffi_add tvm_ffi_cmake_arg_target) Review Comment:  The variable naming for argument parsing is inconsistent between `tvm_ffi_add` and `tvm_ffi_install`. In `tvm_ffi_add`, the positional argument is named `tvm_ffi_cmake_arg_target` and the `cmake_parse_arguments` prefix is `tvm_ffi_cmake_arg_`. In `tvm_ffi_install`, they are `tvm_ffi_install_target` and `tvm_ffi_install_` respectively. For better consistency and readability, consider using a more conventional naming scheme. For example, you could rename the positional argument to `target` and use `tvm_ffi_add_` as the prefix for `cmake_parse_arguments` in this function. This would align it with `tvm_ffi_install` and make it clearer that the parsed variables belong to the `tvm_ffi_add` function. ########## cmake/Utils/Library.cmake: ########## @@ -41,7 +42,7 @@ endfunction () # ~~~ function (tvm_ffi_add_apple_dsymutil target_name) # running dsymutil on macos to generate debugging symbols for backtraces - if (APPLE AND TVM_FFI_USE_LIBBACKTRACE) + if (APPLE) Review Comment:  With the removal of the `TVM_FFI_USE_LIBBACKTRACE` check from this `if` condition, the function's docstring (lines 36-38) is now partially incorrect as it still mentions that the function is a no-op when `libbacktrace` is disabled. Please consider updating the docstring to reflect this change and avoid confusion. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
